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);
}
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
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
<[email protected]>
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)
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
On Tue, Mar 22, 2011 at 2:16 PM, Rajiv Andrade
<[email protected]> 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
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 <[email protected]>
Security Development
IBM Linux Technology Center
+55 19 81095527
-----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 [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-----