Re: [Qemu-devel] [PULL 12/22] sev/i386: add command to create launch memory encryption context

[Peter Maydell]

On 13 March 2018 at 12:56, Paolo Bonzini <address@hidden> wrote:
> From: Brijesh Singh <address@hidden>
>
> The KVM_SEV_LAUNCH_START command creates a new VM encryption key (VEK).
> The encryption key created with the command will be used for encrypting
> the bootstrap images (such as guest bios).

Hi. Coverity points out a resource leak here (CID1390626):

> +static int
> +sev_launch_start(SEVState *s)
> +{
> +    gsize sz;
> +    int ret = 1;
> +    int fw_error;
> +    QSevGuestInfo *sev = s->sev_info;
> +    struct kvm_sev_launch_start *start;
> +    guchar *session = NULL, *dh_cert = NULL;
> +
> +    start = g_new0(struct kvm_sev_launch_start, 1);

Here we allocate memory into start...

> +
> +    start->handle = object_property_get_int(OBJECT(sev), "handle",
> +                                            &error_abort);
> +    start->policy = object_property_get_int(OBJECT(sev), "policy",
> +                                            &error_abort);
> +    if (sev->session_file) {
> +        if (sev_read_file_base64(sev->session_file, &session, &sz) < 0) {

...but here we exit without freeing it...

> +            return 1;
> +        }
> +        start->session_uaddr = (unsigned long)session;
> +        start->session_len = sz;
> +    }
> +
> +    if (sev->dh_cert_file) {
> +        if (sev_read_file_base64(sev->dh_cert_file, &dh_cert, &sz) < 0) {
> +            return 1;

...ditto here...

> +        }
> +        start->dh_uaddr = (unsigned long)dh_cert;
> +        start->dh_len = sz;
> +    }
> +
> +    trace_kvm_sev_launch_start(start->policy, session, dh_cert);
> +    ret = sev_ioctl(s->sev_fd, KVM_SEV_LAUNCH_START, start, &fw_error);
> +    if (ret < 0) {
> +        error_report("%s: LAUNCH_START ret=%d fw_error=%d '%s'",
> +                __func__, ret, fw_error, fw_error_to_str(fw_error));
> +        return 1;

...and here.

> +    }
> +
> +    object_property_set_int(OBJECT(sev), start->handle, "handle",
> +                            &error_abort);
> +    sev_set_guest_state(SEV_STATE_LAUNCH_UPDATE);
> +    s->handle = start->handle;
> +    s->policy = start->policy;
> +
> +    g_free(start);
> +    g_free(session);
> +    g_free(dh_cert);
> +
> +    return 0;
> +}

Coverity doesn't spot it, but I think we will also leak
the memory pointed to by session and dh_cert in some of those
error-exit paths.

thanks
-- PMM