[GIT PULL] TPM driver robustness fixes

[GIT PULL] TPM driver robustness fixes

[Rajiv Andrade]

Hi James,


The following changes since commit 2e270d84223262a38d4755c61d55f5c73ea89e56:

   Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6 (2011-03-16 13:26:17 -0700)

are available in the git repository at:

   git://tpmdd.git.sourceforge.net/gitroot/tpmdd/tpmdd/ for-james

Peter Huewe (3):

       This patch changes the call of tpm_transmit by supplying the size of the userspace buffer instead of TPM_BUFSIZE

       This patch fixes information leakage to the userspace by initializing the data buffer to zero

       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.

  drivers/char/tpm/tpm.c |    7 ++++---

  1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c

index 1f46f1c..c6d2cde 100644

--- a/drivers/char/tpm/tpm.c

+++ b/drivers/char/tpm/tpm.c

@@ -427,7 +427,7 @@ static ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,

      goto out;

  

  out_recv:

-    rc = chip->vendor.recv(chip, (u8 *) buf, bufsiz);

+    rc = chip->vendor.recv(chip, (u8 *) buf, TPM_BUFSIZE);

      if (rc<  0)

          dev_err(chip->dev,

              "tpm_transmit: tpm_recv: error %zd\n", rc);

@@ -980,7 +980,7 @@ int tpm_open(struct inode *inode, struct file *file)

          return -EBUSY;

      }

  

-    chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);

+    chip->data_buffer = kzalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);

      if (chip->data_buffer == NULL) {

          clear_bit(0,&chip->is_open);

          put_device(chip->dev);

@@ -1035,7 +1035,7 @@ ssize_t tpm_write(struct file *file, const char __user *buf,

      }

  

      /* atomic tpm command send and result receive */

-    out_size = tpm_transmit(chip, chip->data_buffer, TPM_BUFSIZE);

+    out_size = tpm_transmit(chip, chip->data_buffer, in_size);

  

      atomic_set(&chip->data_pending, out_size);

      mutex_unlock(&chip->buffer_mutex);

@@ -1064,6 +1064,7 @@ ssize_t tpm_read(struct file *file, char __user *buf,

          mutex_lock(&chip->buffer_mutex);

          if (copy_to_user(buf, chip->data_buffer, ret_size))

              ret_size = -EFAULT;

+        memset(chip->data_buffer, 0, ret_size);

          mutex_unlock(&chip->buffer_mutex);

      }

  

Re: [GIT PULL] TPM driver robustness fixes

[Rajiv Andrade]

On 03/16/2011 11:23 PM, Rajiv Andrade wrote:
> Hi James,
>
>
> The following changes since commit 
> 2e270d84223262a38d4755c61d55f5c73ea89e56:
>
>   Merge branch 'for-linus' of 
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6 (2011-03-16 
> 13:26:17 -0700)
>
> are available in the git repository at:
>
>   git://tpmdd.git.sourceforge.net/gitroot/tpmdd/tpmdd/ for-james
>
> Peter Huewe (3):
>
>       This patch changes the call of tpm_transmit by supplying the 
> size of the userspace buffer instead of TPM_BUFSIZE
>
>       This patch fixes information leakage to the userspace by 
> initializing the data buffer to zero
>
>       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.

Given the nature of these changes, they should go to current linus as 
bug fixes.

Thanks,
Rajiv

Re: [GIT PULL] TPM driver robustness fixes

[James Morris]

On Wed, 16 Mar 2011, Rajiv Andrade wrote:

> Hi James,
> 
> 
> The following changes since commit 2e270d84223262a38d4755c61d55f5c73ea89e56:
> 
>   Merge branch 'for-linus' of
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6 (2011-03-16
> 13:26:17 -0700)
> 
> are available in the git repository at:
> 
>   git://tpmdd.git.sourceforge.net/gitroot/tpmdd/tpmdd/ for-james
> 
> Peter Huewe (3):
> 
>       This patch changes the call of tpm_transmit by supplying the size of the
> userspace buffer instead of TPM_BUFSIZE
> 
>       This patch fixes information leakage to the userspace by initializing
> the data buffer to zero
> 
>       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.

These patches don't have proper subjects.

Also:

                if (copy_to_user(buf, chip->data_buffer, ret_size))
                        ret_size = -EFAULT;
+               memset(chip->data_buffer, 0, ret_size);


Consider what happens in memset if copy_to_user fails.

One of the patches is flagged with "Discussion needed ...", without any 
evidence of that the discussion happened.



- James
-- 
James Morris
<jmorris@namei.org>

Re: [GIT PULL] TPM driver robustness fixes

[Peter Huewe]

Hi James,


> These patches don't have proper subjects.

The patch subjects were somehow mangled as it seems.
The original patch subjects were
- char/tpm: Fix uninitialized usage of data buffer
- char/tpm: Call tpm_transmit with correct size
- char/tpm: Zero buffer after copying to userspace

I've sent them to the kernel security list but as it seems during the
transmission to tpmdd the subjects were stripped.
If desired I can resend them.


> Consider what happens in memset if copy_to_user fails.
Sorry I don't see it. Can you please clearify the problem for me?



> One of the patches is flagged with "Discussion needed ...", without any
> evidence of that the discussion happened.
You're right about that - no discussion happened ;)


Peter
(p.s.: again in plaintext - sorry)

Re: [GIT PULL] TPM driver robustness fixes

[Rajiv Andrade]

On 03/22/2011 09:58 AM, Peter Huewe wrote:
>
>> Consider what happens in memset if copy_to_user fails.
> Sorry I don't see it. Can you please clearify the problem for me?
>
memset() is called with the count parameter being a negative value,
ret_size=-EFAULT.

Rajiv

Re: [GIT PULL] TPM driver robustness fixes

[Peter Huewe]

On Tue, Mar 22, 2011 at 2:16 PM, Rajiv Andrade
<srajiv@linux.vnet.ibm.com> wrote:
> On 03/22/2011 09:58 AM, Peter Huewe wrote:
>>
>>> Consider what happens in memset if copy_to_user fails.
>>
>> Sorry I don't see it. Can you please clearify the problem for me?
>>
> memset() is called with the count parameter being a negative value,
> ret_size=-EFAULT.

You're right - sorry.

Do you prefer a temporary variable like this

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index d184d75..d3976f5 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1051,7 +1051,7 @@ ssize_t tpm_read(struct file *file, char __user *buf,
                 size_t size, loff_t *off)
 {
        struct tpm_chip *chip = file->private_data;
-       ssize_t ret_size;
+       ssize_t ret_size, tmp_size;

        del_singleshot_timer_sync(&chip->user_read_timer);
        flush_work_sync(&chip->work);
@@ -1061,9 +1061,11 @@ ssize_t tpm_read(struct file *file, char __user *buf,
                if (size < ret_size)
                        ret_size = size;

+               tmp_size = ret_size;
                mutex_lock(&chip->buffer_mutex);
                if (copy_to_user(buf, chip->data_buffer, ret_size))
                        ret_size = -EFAULT;
+               memset(chip->data_buffer, 0, tmp_size);
                mutex_unlock(&chip->buffer_mutex);
        }


Or an if else like this:

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index d184d75..571c0ca 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1062,8 +1062,12 @@ 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))
+               if (copy_to_user(buf, chip->data_buffer, ret_size)) {
+                       memset(chip->data_buffer, 0, ret_size);
                        ret_size = -EFAULT;
+               } else {
+                       memset(chip->data_buffer, 0, ret_size);
+               }
                mutex_unlock(&chip->buffer_mutex);
        }


Or rather:
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index d184d75..2f2f65e 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);
        }



Thanks,
Peter

Re: [GIT PULL] TPM driver robustness fixes

[Rajiv Andrade]

On 03/22/2011 01:08 PM, Peter Huewe wrote:
> Or rather:
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index d184d75..2f2f65e 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);
>         }
> 
> 
> 
> Thanks,
> Peter

This approach is definitely the best.

Thanks,
Rajiv


-- 
Thanks,
Rajiv Andrade <srajiv@br.ibm.com>
Security Development
IBM Linux Technology Center
+55 19 81095527

Re: [GIT PULL] TPM driver robustness fixes

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/16/2011 10:23 PM, Rajiv Andrade wrote:
> Hi James,
> 
> 
> The following changes since commit
> 2e270d84223262a38d4755c61d55f5c73ea89e56:
> 
>   Merge branch 'for-linus' of
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6 (2011-03-16
> 13:26:17 -0700)
> 
> are available in the git repository at:
> 
>   git://tpmdd.git.sourceforge.net/gitroot/tpmdd/tpmdd/ for-james
> 
> Peter Huewe (3):
> 
>       This patch changes the call of tpm_transmit by supplying the size
> of the userspace buffer instead of TPM_BUFSIZE
> 
>       This patch fixes information leakage to the userspace by
> initializing the data buffer to zero
> 
>       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.
> 
>  drivers/char/tpm/tpm.c |    7 ++++---
> 
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> 
> index 1f46f1c..c6d2cde 100644
> 
> --- a/drivers/char/tpm/tpm.c
> 
> +++ b/drivers/char/tpm/tpm.c
> 
> @@ -427,7 +427,7 @@ static ssize_t tpm_transmit(struct tpm_chip *chip,
> const char *buf,
> 
>      goto out;
> 
>  
> 
>  out_recv:
> 
> -    rc = chip->vendor.recv(chip, (u8 *) buf, bufsiz);
> 
> +    rc = chip->vendor.recv(chip, (u8 *) buf, TPM_BUFSIZE);
> 
>      if (rc<  0)
> 
>          dev_err(chip->dev,
> 
>              "tpm_transmit: tpm_recv: error %zd\n", rc);
> 

This works in the tpm_write case, but it seems like it's introducing a
bug in the __tpm_pcr_read (bufsiz=30) and tpm_continue_selftest
(bufsiz=10) cases. Am I missing something?

- -Jeff

> @@ -980,7 +980,7 @@ int tpm_open(struct inode *inode, struct file *file)
> 
>          return -EBUSY;
> 
>      }
> 
>  
> 
> -    chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
> 
> +    chip->data_buffer = kzalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
> 
>      if (chip->data_buffer == NULL) {
> 
>          clear_bit(0,&chip->is_open);
> 
>          put_device(chip->dev);
> 
> @@ -1035,7 +1035,7 @@ ssize_t tpm_write(struct file *file, const char
> __user *buf,
> 
>      }
> 
>  
> 
>      /* atomic tpm command send and result receive */
> 
> -    out_size = tpm_transmit(chip, chip->data_buffer, TPM_BUFSIZE);
> 
> +    out_size = tpm_transmit(chip, chip->data_buffer, in_size);
> 
>  
> 
>      atomic_set(&chip->data_pending, out_size);
> 
>      mutex_unlock(&chip->buffer_mutex);
> 
> @@ -1064,6 +1064,7 @@ ssize_t tpm_read(struct file *file, char __user *buf,
> 
>          mutex_lock(&chip->buffer_mutex);
> 
>          if (copy_to_user(buf, chip->data_buffer, ret_size))
> 
>              ret_size = -EFAULT;
> 
> +        memset(chip->data_buffer, 0, ret_size);
> 
>          mutex_unlock(&chip->buffer_mutex);
> 
>      }
> 
>  
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


- -- 
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/

iEYEARECAAYFAk2naJsACgkQLPWxlyuTD7JMoQCaAj3aOiGYFubu/NKHRT0bM4Fy
wwkAn2Zj2yH2KLFrA0otJZ3ZSOZttd9B
=cxp4
-----END PGP SIGNATURE-----

Re: [GIT PULL] TPM driver robustness fixes

[Eugene Teo]

Hi Rajiv,

On Thu, Mar 17, 2011 at 10:23 AM, Rajiv Andrade
<srajiv@linux.vnet.ibm.com> wrote:
> Hi James,
>
> The following changes since commit 2e270d84223262a38d4755c61d55f5c73ea89e56:
>
>  Merge branch 'for-linus' of
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6 (2011-03-16
> 13:26:17 -0700)
>
> are available in the git repository at:
>
>  git://tpmdd.git.sourceforge.net/gitroot/tpmdd/tpmdd/ for-james
>
> Peter Huewe (3):

I think this one fell through the cracks.

Out of the three patches, only one is committed upstream:
[PATCH 1/3] char/tpm: Fix uninitialized usage of data buffer
1309d7afbed112f0e8e90be9af975550caa0076b

What happened to the other two?
[PATCH 2/3] char/tpm: Call tpm_transmit with correct size
[PATCH 3/3] char/tpm: zero buffer after copying to userspace

http://tpmdd.git.sourceforge.net/git/gitweb.cgi?p=tpmdd/tpmdd git repo
is gone too?

Eugene