Re: [PULL 21/45] i386/sev: Introduce 'sev-snp-guest' object

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 4 Jun 2024 at 07:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Brijesh Singh <brijesh.singh@amd.com>
>
> SEV-SNP support relies on a different set of properties/state than the
> existing 'sev-guest' object. This patch introduces the 'sev-snp-guest'
> object, which can be used to configure an SEV-SNP guest. For example,
> a default-configured SEV-SNP guest with no additional information
> passed in for use with attestation:
>
>   -object sev-snp-guest,id=sev0
>
> or a fully-specified SEV-SNP guest where all spec-defined binary
> blobs are passed in as base64-encoded strings:
>
>   -object sev-snp-guest,id=sev0, \
>     policy=0x30000, \
>     init-flags=0, \
>     id-block=YWFhYWFhYWFhYWFhYWFhCg==, \
>     id-auth=CxHK/OKLkXGn/KpAC7Wl1FSiisWDbGTEKz..., \
>     author-key-enabled=on, \
>     host-data=LNkCWBRC5CcdGXirbNUV1OrsR28s..., \
>     guest-visible-workarounds=AA==, \
>
> See the QAPI schema updates included in this patch for more usage
> details.
>
> In some cases these blobs may be up to 4096 characters, but this is
> generally well below the default limit for linux hosts where
> command-line sizes are defined by the sysconf-configurable ARG_MAX
> value, which defaults to 2097152 characters for Ubuntu hosts, for
> example.

Hi; Coverity reports (CID 1546887) an issue in this code:

> +static void
> +sev_snp_guest_set_id_block(Object *obj, const char *value, Error **errp)
> +{
> +    SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(obj);
> +    struct kvm_sev_snp_launch_finish *finish = &sev_snp_guest->kvm_finish_conf;
> +    gsize len;
> +
> +    g_free(sev_snp_guest->id_block);
> +    g_free((guchar *)finish->id_block_uaddr);
> +
> +    /* store the base64 str so we don't need to re-encode in getter */
> +    sev_snp_guest->id_block = g_strdup(value);
> +
> +    finish->id_block_uaddr =
> +        (uint64_t)qbase64_decode(sev_snp_guest->id_block, -1, &len, errp);
> +
> +    if (!finish->id_block_uaddr) {
> +        return;
> +    }
> +
> +    if (len != KVM_SEV_SNP_ID_BLOCK_SIZE) {
> +        error_setg(errp, "parameter length of %lu not equal to %u",
> +                   len, KVM_SEV_SNP_ID_BLOCK_SIZE);
> +        return;

Here if len is not 96 then we return early...

> +    }
> +
> +    finish->id_block_en = (len) ? 1 : 0;

...but here we check whether len is 0, which it can never be.

What was the intention here?

Side notes: you don't need to cast the argument to g_free(); and
you don't need to put brackets around a single argument like "len".

thanks
-- PMM