Re: [PATCH v3] i386/sev: Ensure attestation report length is valid before retrieving

["Philippe Mathieu-Daudé" <philippe.mathieu.daude@gmail.com>]

On 4/3/22 21:11, Tyler Fanelli wrote:
> The length of the attestation report buffer is never checked to be
> valid before allocation is made. If the length of the report is returned
> to be 0, the buffer to retrieve the attestation buffer is allocated with
> length 0 and passed to the kernel to fill with contents of the attestation
> report. Leaving this unchecked is dangerous and could lead to undefined
> behavior.
> 
> Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
> ---
>   target/i386/sev.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 025ff7a6f8..e82be3e350 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -616,6 +616,8 @@ static SevAttestationReport *sev_get_attestation_report(const char *mnonce,
>           return NULL;
>       }
>   
> +    input.len = 0;

I agree with Daniel's review of your v1:

   "The declaration of 'input' already zero initializes."

https://lore.kernel.org/qemu-devel/YiJi9IYqtZvNQIRc@redhat.com/

>       /* Query the report length */
>       ret = sev_ioctl(sev->sev_fd, KVM_SEV_GET_ATTESTATION_REPORT,
>               &input, &err);
> @@ -626,6 +628,11 @@ static SevAttestationReport *sev_get_attestation_report(const char *mnonce,
>                          ret, err, fw_error_to_str(err));
>               return NULL;
>           }
> +    } else if (input.len == 0) {
> +        error_setg(errp, "SEV: Failed to query attestation report:"
> +                         " length returned=%u",
> +                   input.len);
> +        return NULL;
>       }
>   
>       data = g_malloc(input.len);