RE: [PATCH v2] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent kernel memory leak

[Kalra, Ashish]

[AMD Official Use Only - General]

Hello Sean,

-----Original Message-----
From: Sean Christopherson <seanjc@xxxxxxxxxx> 
Sent: Monday, May 16, 2022 12:43 PM
To: Kalra, Ashish <Ashish.Kalra@xxxxxxx>
Cc: Peter Gonda <pgonda@xxxxxxxxxx>; Allen, John <John.Allen@xxxxxxx>; Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>; Linux Crypto Mailing List <linux-crypto@xxxxxxxxxxxxxxx>; Lendacky, Thomas <Thomas.Lendacky@xxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>; Andy Nguyen <theflow@xxxxxxxxxx>; David Rientjes <rientjes@xxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v2] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent kernel memory leak

On Mon, May 16, 2022, Kalra, Ashish wrote:
> > >Would it be safer to memset @data here to all zeros too?
> >
> > It will be, but this command/function is safe as firmware will fill 
> > in the whole buffer here with the PLATFORM STATUS data retuned to the user.
> 
> > That does seem safe for now but I thought we decided it would be 
> > prudent to not trust the PSPs implementation here and clear all the 
> > buffers that eventually get sent to userspace?
> 
> Yes, but the issue is when the user programs a buffer size larger the 
> one filled in by the firmware. In this case firmware is always going 
> to fill up the whole buffer with PLATFORM_STATUS data, so it will be 
> always be safe. The issue is mainly with the kernel side doing a 
> copy_to_user() based on user programmed length instead of the firmware returned buffer length.

>Peter's point is that it costs the kernel very little to be paranoid and not make assumptions about whether or not the PSP will fill an entire struct as expected.

>I agree it feels a bit silly since all fields are output, but on the other hand the PSP spec just says:

>  The following data structure is written to memory at STATUS_PADDR

>and the data structure has several reserved fields.  I don't love assuming that the PSP will always write zeros for the reserved fields and not do something like:

>	if (rmp_initialized)
>		data[3] |= IS_RMP_INIT;
>	else
>		data[3] &= ~IS_RMP_INIT;

>Given that zeroing @data in the kernel is easy and this is not a hot patch, I prefer the paranoid approach unless the PSP spec is much more explicit in saying that it writes all bits and bytes on success.

I agree with that and we will resend a v3 of the crypto patch with this change added.

Thanks,
Ashish