Re: [Qemu-devel] [PULL 10/22] sev/i386: add command to initialize the memory encryption context

[Peter Maydell]

On 13 March 2018 at 12:56, Paolo Bonzini <address@hidden> wrote:
> From: Brijesh Singh <address@hidden>
>
> When memory encryption is enabled, KVM_SEV_INIT command is used to
> initialize the platform. The command loads the SEV related persistent
> data from non-volatile storage and initializes the platform context.
> This command should be first issued before invoking any other guest
> commands provided by the SEV firmware.

Hi; Coverity points out a memory leak in this code
(CID 1390613):

> +void *
> +sev_guest_init(const char *id)
> +{
> +    SEVState *s;
> +    char *devname;
> +    int ret, fw_error;
> +    uint32_t ebx;
> +    uint32_t host_cbitpos;
> +    struct sev_user_data_status status = {};
> +
> +    s = g_new0(SEVState, 1);

Here we allocate memory into 's'...

> +    s->sev_info = lookup_sev_guest_info(id);
> +    if (!s->sev_info) {
> +        error_report("%s: '%s' is not a valid '%s' object",
> +                     __func__, id, TYPE_QSEV_GUEST_INFO);
> +        goto err;

...and this error-exit path will not free it because
it does g_free(sev_state), not g_free(s).

> +    }
> +
> +    sev_state = s;
> +    s->state = SEV_STATE_UNINIT;

[...]

> +err:
> +    g_free(sev_state);
> +    sev_state = NULL;
> +    return NULL;
> +}

The simplest fix is probably to move the 'sev_state = s;
s->state = SEV_STATE_UNINIT;' assignments to earlier in the
function.

Cleaner might be to ensure that nothing in the rest of
the function depends on sev_state being initialized,
and then to work purely with 's' and only set sev_state
to s just before returning success.

thanks
-- PMM