TPM driver robustness fixes

GIT PULL Rajiv Andrade 5 participants 9 messages Mar 17, 2011 – Jul 19, 2011 1 message not in the archive TPM device driver

lore

Share & tools

lore

Rajiv Andrade

author

2011-03-17 02:23 UTC #0 lore raw

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

drivers/char/tpm/tpm.c +0 -0

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

     }

Rajiv Andrade

author

2011-03-17 02:26 UTC #1 lore raw

Quoted 14 lines · 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):

Quoted 8 lines

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

James Morris

2011-03-21 23:01 UTC #2 lore raw

In reply to Rajiv Andrade

Quoted 15 lines · 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):

Quoted 8 lines

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

Signature · 2 lines

James Morris
<[email protected]>

A message that is not in the archive (sent off-list or not yet received): [email protected]

Peter Huewe

2011-03-22 12:58 UTC #4 lore raw

Hi James,

Quoted 1 line

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.

Quoted 1 line

Consider what happens in memset if copy_to_user fails.

Sorry I don't see it. Can you please clearify the problem for me?

Quoted 2 lines

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)

Rajiv Andrade

author

2011-03-22 13:16 UTC #5 lore raw

Quoted 4 lines · Peter Huewe

Quoted 2 lines
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

Peter Huewe

2011-03-22 16:08 UTC #6 lore raw

Quoted 8 lines · Rajiv Andrade

Quoted 5 lines · Peter Huewe
Quoted 2 lines
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

drivers/char/tpm/tpm.c +3 -1

index d184d75..d3976f5 100644
@@ -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:

drivers/char/tpm/tpm.c +5 -1

index d184d75..571c0ca 100644
@@ -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:

drivers/char/tpm/tpm.c +5 -1

index d184d75..2f2f65e 100644
@@ -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

Rajiv Andrade

author

2011-03-28 16:07 UTC #7 lore raw

Quoted 28 lines · Peter Huewe

Or rather:

drivers/char/tpm/tpm.c +5 -1

index d184d75..2f2f65e 100644
@@ -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);
        }

Quoted 2 lines

Thanks,
Peter

This approach is definitely the best.

Thanks,
Rajiv

Signature · 5 lines

Thanks,
Rajiv Andrade <[email protected]>
Security Development
IBM Linux Technology Center
+55 19 81095527

Jeff Mahoney

2011-04-14 21:35 UTC #8 lore raw

In reply to Rajiv Andrade

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

Quoted 46 lines · 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(-)
drivers/char/tpm/tpm.c +0 -0
index 1f46f1c..c6d2cde 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
drivers/char/tpm/tpm.c +0 -0
@@ -427,7 +427,7 @@ static ssize_t tpm_transmit(struct tpm_chip *chip,
 const char *buf,
 
     goto out;
 
 
 
 out_recv:

drivers/char/tpm/tpm.c static ssize_t tpm_transmit(struct tpm_chip *chip,

Quoted 10 lines

drivers/char/tpm/tpm.c +1 -1

 
-    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

Quoted 58 lines

drivers/char/tpm/tpm.c +3 -2

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

Signature · 5 lines

-- 
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
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-----

Eugene Teo

2011-07-19 06:01 UTC #9 lore raw

In reply to Rajiv Andrade

Hi Rajiv,

Quoted 8 lines · 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)

Quoted 5 lines

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