lkml.org 
[lkml]   [2022]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v2] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent kernel memory leak
Date
[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

\
 
 \ /
  Last update: 2022-05-16 20:12    [W:2.219 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site