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

Submitted by Andy Whitcroft on Dec. 5, 2011, 4:18 p.m.

Details

Message ID 1323101922-9653-4-git-send-email-apw@canonical.com
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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);
 	}