[PATCH v2] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent kernel memory leak
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@vger.kernel.org Reported-by: Andy Nguyen theflow@google.com Suggested-by: David Rientjes rientjes@google.com Suggested-by: Peter Gonda pgonda@google.com Signed-off-by: John Allen john.allen@amd.com --- v2: - Add fixes tags and CC stable@vger.kernel.org --- drivers/crypto/ccp/sev-dev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 6ab93dfd478a..e2298843ea8a 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -604,7 +604,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable) if (input.length > SEV_FW_BLOB_MAX_SIZE) return -EFAULT;
- blob = kmalloc(input.length, GFP_KERNEL); + blob = kzalloc(input.length, GFP_KERNEL); if (!blob) return -ENOMEM;
@@ -828,7 +828,7 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp) input_address = (void __user *)input.address;
if (input.address && input.length) { - id_blob = kmalloc(input.length, GFP_KERNEL); + id_blob = kzalloc(input.length, GFP_KERNEL); if (!id_blob) return -ENOMEM;
@@ -947,14 +947,14 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable) if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE) return -EFAULT;
- pdh_blob = kmalloc(input.pdh_cert_len, GFP_KERNEL); + pdh_blob = kzalloc(input.pdh_cert_len, GFP_KERNEL); if (!pdh_blob) return -ENOMEM;
data.pdh_cert_address = __psp_pa(pdh_blob); data.pdh_cert_len = input.pdh_cert_len;
- cert_blob = kmalloc(input.cert_chain_len, GFP_KERNEL); + cert_blob = kzalloc(input.cert_chain_len, GFP_KERNEL); if (!cert_blob) { ret = -ENOMEM; goto e_free_pdh;
On Mon, May 16, 2022 at 8:46 AM John Allen john.allen@amd.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@vger.kernel.org Reported-by: Andy Nguyen theflow@google.com Suggested-by: David Rientjes rientjes@google.com Suggested-by: Peter Gonda pgonda@google.com Signed-off-by: John Allen john.allen@amd.com
v2:
- Add fixes tags and CC stable@vger.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?
[AMD Official Use Only - General]
Hello Peter,
-----Original Message----- From: Peter Gonda pgonda@google.com Sent: Monday, May 16, 2022 10:53 AM To: Allen, John John.Allen@amd.com Cc: Herbert Xu herbert@gondor.apana.org.au; Linux Crypto Mailing List linux-crypto@vger.kernel.org; Sean Christopherson seanjc@google.com; Lendacky, Thomas Thomas.Lendacky@amd.com; Kalra, Ashish Ashish.Kalra@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 at 8:46 AM John Allen john.allen@amd.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@vger.kernel.org Reported-by: Andy Nguyen theflow@google.com Suggested-by: David Rientjes rientjes@google.com Suggested-by: Peter Gonda pgonda@google.com Signed-off-by: John Allen john.allen@amd.com
v2:
- Add fixes tags and CC stable@vger.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.
Thanks, Ashish
On Mon, May 16, 2022 at 9:02 AM Kalra, Ashish Ashish.Kalra@amd.com wrote:
[AMD Official Use Only - General]
Hello Peter,
-----Original Message----- From: Peter Gonda pgonda@google.com Sent: Monday, May 16, 2022 10:53 AM To: Allen, John John.Allen@amd.com Cc: Herbert Xu herbert@gondor.apana.org.au; Linux Crypto Mailing List linux-crypto@vger.kernel.org; Sean Christopherson seanjc@google.com; Lendacky, Thomas Thomas.Lendacky@amd.com; Kalra, Ashish Ashish.Kalra@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 at 8:46 AM John Allen john.allen@amd.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@vger.kernel.org Reported-by: Andy Nguyen theflow@google.com Suggested-by: David Rientjes rientjes@google.com Suggested-by: Peter Gonda pgonda@google.com Signed-off-by: John Allen john.allen@amd.com
v2:
- Add fixes tags and CC stable@vger.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?
Thanks, Ashish
[AMD Official Use Only - General]
Hello Peter,
-----Original Message----- From: Peter Gonda pgonda@google.com Sent: Monday, May 16, 2022 12:13 PM To: Kalra, Ashish Ashish.Kalra@amd.com Cc: Allen, John John.Allen@amd.com; Herbert Xu herbert@gondor.apana.org.au; Linux Crypto Mailing List linux-crypto@vger.kernel.org; Sean Christopherson seanjc@google.com; 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 at 9:02 AM Kalra, Ashish Ashish.Kalra@amd.com wrote:
[AMD Official Use Only - General]
Hello Peter,
-----Original Message----- From: Peter Gonda pgonda@google.com Sent: Monday, May 16, 2022 10:53 AM To: Allen, John John.Allen@amd.com Cc: Herbert Xu herbert@gondor.apana.org.au; Linux Crypto Mailing List linux-crypto@vger.kernel.org; Sean Christopherson seanjc@google.com; Lendacky, Thomas Thomas.Lendacky@amd.com; Kalra, Ashish Ashish.Kalra@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 at 8:46 AM John Allen john.allen@amd.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@vger.kernel.org Reported-by: Andy Nguyen theflow@google.com Suggested-by: David Rientjes rientjes@google.com Suggested-by: Peter Gonda pgonda@google.com Signed-off-by: John Allen john.allen@amd.com
v2:
- Add fixes tags and CC stable@vger.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
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.
[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
linux-stable-mirror@lists.linaro.org
-
John Allen -
Kalra, Ashish -
Peter Gonda -
Sean Christopherson