Messages in this thread | | | From | "Kalra, Ashish" <> | Subject | RE: [PATCH v2] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent kernel memory leak | Date | Mon, 16 May 2022 18:11:02 +0000 |
| |
[AMD Official Use Only - General]
Hello Sean,
-----Original Message----- From: Sean Christopherson <seanjc@google.com> Sent: Monday, May 16, 2022 12:43 PM To: Kalra, Ashish <Ashish.Kalra@amd.com> Cc: Peter Gonda <pgonda@google.com>; Allen, John <John.Allen@amd.com>; Herbert Xu <herbert@gondor.apana.org.au>; Linux Crypto Mailing List <linux-crypto@vger.kernel.org>; Lendacky, Thomas <Thomas.Lendacky@amd.com>; LKML <linux-kernel@vger.kernel.org>; Andy Nguyen <theflow@google.com>; David Rientjes <rientjes@google.com>; stable@vger.kernel.org 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
| |