Re: [PULL 18/45] i386/sev: Introduce "sev-common" type to encapsulate common SEV state

[Peter Maydell]

On Tue, 4 Jun 2024 at 07:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Michael Roth <michael.roth@amd.com>
>
> Currently all SEV/SEV-ES functionality is managed through a single
> 'sev-guest' QOM type. With upcoming support for SEV-SNP, taking this
> same approach won't work well since some of the properties/state
> managed by 'sev-guest' is not applicable to SEV-SNP, which will instead
> rely on a new QOM type with its own set of properties/state.
>
> To prepare for this, this patch moves common state into an abstract
> 'sev-common' parent type to encapsulate properties/state that are
> common to both SEV/SEV-ES and SEV-SNP, leaving only SEV/SEV-ES-specific
> properties/state in the current 'sev-guest' type. This should not
> affect current behavior or command-line options.
>
> As part of this patch, some related changes are also made:
>
>   - a static 'sev_guest' variable is currently used to keep track of
>     the 'sev-guest' instance. SEV-SNP would similarly introduce an
>     'sev_snp_guest' static variable. But these instances are now
>     available via qdev_get_machine()->cgs, so switch to using that
>     instead and drop the static variable.
>
>   - 'sev_guest' is currently used as the name for the static variable
>     holding a pointer to the 'sev-guest' instance. Re-purpose the name
>     as a local variable referring the 'sev-guest' instance, and use
>     that consistently throughout the code so it can be easily
>     distinguished from sev-common/sev-snp-guest instances.
>
>   - 'sev' is generally used as the name for local variables holding a
>     pointer to the 'sev-guest' instance. In cases where that now points
>     to common state, use the name 'sev_common'; in cases where that now
>     points to state specific to 'sev-guest' instance, use the name
>     'sev_guest'
>
> In order to enable kernel-hashes for SNP, pull it from
> SevGuestProperties to its parent SevCommonProperties so
> it will be available for both SEV and SNP.

Hi; Coverity points out a problem in this code (CID 1546885):

> @@ -540,12 +491,21 @@ static SevCapability *sev_get_capabilities(Error **errp)
>          return NULL;
>      }
>
> -    fd = open(DEFAULT_SEV_DEVICE, O_RDWR);
> +    sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
> +    if (!sev_common) {
> +        error_setg(errp, "SEV is not configured");

Here we check for a NULL pointer, but we forget to "return",
so execution will continue on...

> +    }
> +
> +    sev_device = object_property_get_str(OBJECT(sev_common), "sev-device",
> +                                         &error_abort);

...and QEMU will crash here when object_property_get_str()
dereferences the NULL pointer.

Adding a "return NULL;" should fix this.

thanks
-- PMM