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 for Android: free password hash cracker in your pocket

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

Message-ID: <SN6PR12MB2767B4A3919E38C7F429CC2D8ECF9@SN6PR12MB2767.namprd12.prod.outlook.com>
Date:   Mon, 16 May 2022 17:24:57 +0000
From:   "Kalra, Ashish" <Ashish.Kalra@....com>
To:     Peter Gonda <pgonda@...gle.com>
CC:     "Allen, John" <John.Allen@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Sean Christopherson <seanjc@...gle.com>,
        "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 Peter,

-----Original Message-----
From: Peter Gonda <pgonda@...gle.com> 
Sent: Monday, May 16, 2022 12:13 PM
To: Kalra, Ashish <Ashish.Kalra@....com>
Cc: Allen, John <John.Allen@....com>; Herbert Xu <herbert@...dor.apana.org.au>; Linux Crypto Mailing List <linux-crypto@...r.kernel.org>; Sean Christopherson <seanjc@...gle.com>; 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 at 9:02 AM Kalra, Ashish <Ashish.Kalra@....com> wrote:
>
> [AMD Official Use Only - General]
>
> Hello Peter,
>
> -----Original Message-----
> From: Peter Gonda <pgonda@...gle.com>
> Sent: Monday, May 16, 2022 10:53 AM
> To: Allen, John <John.Allen@....com>
> Cc: Herbert Xu <herbert@...dor.apana.org.au>; Linux Crypto Mailing 
> List <linux-crypto@...r.kernel.org>; Sean Christopherson 
> <seanjc@...gle.com>; Lendacky, Thomas <Thomas.Lendacky@....com>; 
> Kalra, Ashish <Ashish.Kalra@....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 at 8:46 AM John Allen <john.allen@....com> wrote:
> >
> > For some sev ioctl interfaces, input may be passed that is less than 
> > or equal to SEV_FW_BLOB_MAX_SIZE, but larger than the data that PSP 
> > firmware returns. In this case, kmalloc will allocate memory that is 
> > the size of the input rather than the size of the data. Since PSP 
> > firmware doesn't fully overwrite the buffer, the sev ioctl 
> > interfaces with the issue may return uninitialized slab memory.
> >
> > Currently, all of the ioctl interfaces in the ccp driver are safe, 
> > but to prevent future problems, change all ioctl interfaces that 
> > allocate memory with kmalloc to use kzalloc.
> >
> > Fixes: e799035609e15 ("crypto: ccp: Implement SEV_PEK_CSR ioctl
> > command")
> > Fixes: 76a2b524a4b1d ("crypto: ccp: Implement SEV_PDH_CERT_EXPORT 
> > ioctl command")
> > Fixes: d6112ea0cb344 ("crypto: ccp - introduce SEV_GET_ID2 command")
> > Cc: stable@...r.kernel.org
> > Reported-by: Andy Nguyen <theflow@...gle.com>
> > Suggested-by: David Rientjes <rientjes@...gle.com>
> > Suggested-by: Peter Gonda <pgonda@...gle.com>
> > Signed-off-by: John Allen <john.allen@....com>
> > ---
> > v2:
> >   - Add fixes tags and CC stable@...r.kernel.org
> > ---
>
>
> >static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp) { 
> >struct sev_user_data_status data; int ret;
>
> >ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, 
> >&argp->error); if (ret) return ret;
>
> >if (copy_to_user((void __user *)argp->data, &data, sizeof(data))) ret 
> >= -EFAULT;
>
> >return ret;
> >}
>
> >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.

Thanks,
Ashish