Patchwork [natty,natty/ti-omap4,CVE,1/1] TPM: Zero buffer after copying to userspace

login
register
mail settings
Submitter Andy Whitcroft
Date Dec. 5, 2011, 4:18 p.m.
Message ID <1323101922-9653-4-git-send-email-apw@canonical.com>
Download mbox | patch
Permalink /patch/129348/
State New
Headers show

Comments

Andy Whitcroft - Dec. 5, 2011, 4:18 p.m.
From: Peter Huewe <huewe.external.infineon@googlemail.com>

Since the buffer might contain security related data it might be a good idea to
zero the buffer after we have copied it to userspace.

This got assigned CVE-2011-1162.

Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Cc: Stable Kernel <stable@kernel.org>
Signed-off-by: James Morris <jmorris@namei.org>

(cherry picked from commit 3321c07ae5068568cd61ac9f4ba749006a7185c9)
CVE-2011-1162
BugLink: http://bugs.launchpad.net/bugs/899463
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/char/tpm/tpm.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
Seth Forshee - Dec. 5, 2011, 4:37 p.m.
On Mon, Dec 05, 2011 at 04:18:42PM +0000, Andy Whitcroft wrote:
> From: Peter Huewe <huewe.external.infineon@googlemail.com>
> 
> Since the buffer might contain security related data it might be a good idea to
> zero the buffer after we have copied it to userspace.
> 
> This got assigned CVE-2011-1162.
> 
> Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> Cc: Stable Kernel <stable@kernel.org>
> Signed-off-by: James Morris <jmorris@namei.org>
> 
> (cherry picked from commit 3321c07ae5068568cd61ac9f4ba749006a7185c9)
> CVE-2011-1162
> BugLink: http://bugs.launchpad.net/bugs/899463
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  drivers/char/tpm/tpm.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 7beb0e2..f59dc23 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -1052,6 +1052,7 @@ ssize_t tpm_read(struct file *file, char __user *buf,
>  {
>  	struct tpm_chip *chip = file->private_data;
>  	ssize_t ret_size;
> +	int rc;
>  
>  	del_singleshot_timer_sync(&chip->user_read_timer);
>  	flush_work_sync(&chip->work);
> @@ -1062,8 +1063,11 @@ ssize_t tpm_read(struct file *file, char __user *buf,
>  			ret_size = size;
>  
>  		mutex_lock(&chip->buffer_mutex);
> -		if (copy_to_user(buf, chip->data_buffer, ret_size))
> +		rc = copy_to_user(buf, chip->data_buffer, ret_size);
> +		memset(chip->data_buffer, 0, ret_size);

I realize this is the same as in the upstream commit. But ...

Just before the context qouted here, tpm_read gets the amount of data in
chip->data_buffer and stores it in ret_size, but then limits ret_size
based off of the size of the user buffer. So potentially there could be
data in the buffer that isn't getting zeroed out. Seems like an
incomplete fix to me.

Unrelated to this patch, I also think tpm_read and tpm_write race on the
data buffer. I think tpm_read ought not set chip->data_pending to 0
until it has finished pulling the data out of the buffer and zeroing it
out.

> +		if (rc)
>  			ret_size = -EFAULT;
> +
>  		mutex_unlock(&chip->buffer_mutex);
>  	}
>  
> -- 
> 1.7.5.4
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Patch

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 7beb0e2..f59dc23 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1052,6 +1052,7 @@  ssize_t tpm_read(struct file *file, char __user *buf,
 {
 	struct tpm_chip *chip = file->private_data;
 	ssize_t ret_size;
+	int rc;
 
 	del_singleshot_timer_sync(&chip->user_read_timer);
 	flush_work_sync(&chip->work);
@@ -1062,8 +1063,11 @@  ssize_t tpm_read(struct file *file, char __user *buf,
 			ret_size = size;
 
 		mutex_lock(&chip->buffer_mutex);
-		if (copy_to_user(buf, chip->data_buffer, ret_size))
+		rc = copy_to_user(buf, chip->data_buffer, ret_size);
+		memset(chip->data_buffer, 0, ret_size);
+		if (rc)
 			ret_size = -EFAULT;
+
 		mutex_unlock(&chip->buffer_mutex);
 	}