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

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

[John Allen]

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 and memset the data buffer to zero in
sev_ioctl_do_platform_status.

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
v3:
  - memset data buffer to zero in sev_ioctl_do_platform_status
---
 drivers/crypto/ccp/sev-dev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 6ab93dfd478a..da143cc3a8f5 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -551,6 +551,8 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
 	struct sev_user_data_status data;
 	int ret;
 
+	memset(&data, 0, sizeof(data));
+
 	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, &argp->error);
 	if (ret)
 		return ret;
@@ -604,7 +606,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 +830,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 +949,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;
-- 
2.34.1

Re: [PATCH v3] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent kernel memory leak

[Tom Lendacky]

On 5/17/22 10:39, John Allen 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 and memset the data buffer to zero in
> sev_ioctl_do_platform_status.
> 
> 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
> v3:
>    - memset data buffer to zero in sev_ioctl_do_platform_status
> ---
>   drivers/crypto/ccp/sev-dev.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 6ab93dfd478a..da143cc3a8f5 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -551,6 +551,8 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
>   	struct sev_user_data_status data;
>   	int ret;
>   
> +	memset(&data, 0, sizeof(data));
> +

Probably needs an additional Fixes: tag now?

Fixes: 38103671aad3 ("crypto: ccp: Use the stack and common buffer for status commands")

Thanks,
Tom

>   	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, &argp->error);
>   	if (ret)
>   		return ret;
> @@ -604,7 +606,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 +830,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 +949,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;

Re: [PATCH v3] crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent kernel memory leak

[John Allen]

On Tue, May 17, 2022 at 10:50:08AM -0500, Tom Lendacky wrote:
> On 5/17/22 10:39, John Allen 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 and memset the data buffer to zero in
> > sev_ioctl_do_platform_status.
> > 
> > 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
> > v3:
> >    - memset data buffer to zero in sev_ioctl_do_platform_status
> > ---
> >   drivers/crypto/ccp/sev-dev.c | 10 ++++++----
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 6ab93dfd478a..da143cc3a8f5 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -551,6 +551,8 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
> >   	struct sev_user_data_status data;
> >   	int ret;
> > +	memset(&data, 0, sizeof(data));
> > +
> 
> Probably needs an additional Fixes: tag now?
> 
> Fixes: 38103671aad3 ("crypto: ccp: Use the stack and common buffer for status commands")

Thanks, Tom. I'll add the additional tag in the next revision.

Thanks,
John

> 
> Thanks,
> Tom
> 
> >   	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, &argp->error);
> >   	if (ret)
> >   		return ret;
> > @@ -604,7 +606,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 +830,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 +949,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;