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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <SN6PR12MB276737E018DBCA12EF6EB3218ECF9@SN6PR12MB2767.namprd12.prod.outlook.com>
Date:   Mon, 16 May 2022 18:11:02 +0000
From:   "Kalra, Ashish" <Ashish.Kalra@....com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     Peter Gonda <pgonda@...gle.com>,
        "Allen, John" <John.Allen@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        "Lendacky, Thomas" <Thomas.Lendacky@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        Andy Nguyen <theflow@...gle.com>,
        David Rientjes <rientjes@...gle.com>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH v2] crypto: ccp - Use kzalloc for sev ioctl interfaces to
 prevent kernel memory leak

[AMD Official Use Only - General]

Hello Sean,

-----Original Message-----
From: Sean Christopherson <seanjc@...gle.com> 
Sent: Monday, May 16, 2022 12:43 PM
To: Kalra, Ashish <Ashish.Kalra@....com>
Cc: Peter Gonda <pgonda@...gle.com>; Allen, John <John.Allen@....com>; Herbert Xu <herbert@...dor.apana.org.au>; Linux Crypto Mailing List <linux-crypto@...r.kernel.org>; Lendacky, Thomas <Thomas.Lendacky@....com>; LKML <linux-kernel@...r.kernel.org>; Andy Nguyen <theflow@...gle.com>; David Rientjes <rientjes@...gle.com>; stable@...r.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