[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
