On Tue, 4 Jun 2024 at 07:45, Paolo Bonzini <[email protected]> wrote:
>
> From: Brijesh Singh <[email protected]>
>
> 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
