[PATCH 0/3] TPM: CVE patch, close a race, atomic cleanup

[PATCH 0/3] TPM: CVE patch, close a race, atomic cleanup

[Tim Gardner]

This patch series fixes an oversight in the implementation
of CVE-2011-1162 introduced by 3321c07ae5068568cd61ac9f4ba749006a7185c9,
corrects a race between readers and writers, and cleans up the
use of an unnecessary atomic variable.

Tim Gardner (3):
  TPM: Zero buffer whole after copying to userspace
  TPM: Close data_pending and data_buffer races
  TPM: data_pending is no longer atomic

 drivers/char/tpm/tpm.c |   28 +++++++++++++++-------------
 drivers/char/tpm/tpm.h |    2 +-
 2 files changed, 16 insertions(+), 14 deletions(-)

[PATCH 1/3] TPM: Zero buffer whole after copying to userspace

[Tim Gardner]

Commit 3321c07ae5068568cd61ac9f4ba749006a7185c9 correctly clears the TPM
buffer if the user specified read length is >= the TPM buffer length. However,
if the user specified read length is < the TPM buffer length, then part of the
TPM buffer is left uncleared.

Reported-by: Seth Forshee <seth.forshee@canonical.com>
Cc: Debora Velarde <debora@linux.vnet.ibm.com>
Cc: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Cc: Marcel Selhorst <m.selhorst@sirrix.com>
Cc: tpmdd-devel@lists.sourceforge.net
Cc: stable@vger.kernel.org
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 drivers/char/tpm/tpm.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 361a1df..b366b34 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1115,12 +1115,13 @@ ssize_t tpm_read(struct file *file, char __user *buf,
 	ret_size = atomic_read(&chip->data_pending);
 	atomic_set(&chip->data_pending, 0);
 	if (ret_size > 0) {	/* relay data */
+		ssize_t orig_ret_size = ret_size;
 		if (size < ret_size)
 			ret_size = size;
 
 		mutex_lock(&chip->buffer_mutex);
 		rc = copy_to_user(buf, chip->data_buffer, ret_size);
-		memset(chip->data_buffer, 0, ret_size);
+		memset(chip->data_buffer, 0, orig_ret_size);
 		if (rc)
 			ret_size = -EFAULT;
 
-- 
1.7.0.4

[PATCH 2/3] TPM: Close data_pending and data_buffer races

[Tim Gardner]

There is a race betwen tpm_read() and tpm_write where both chip->data_pending
and chip->data_buffer can be changed by tpm_write() when tpm_read()
clears chip->data_pending, but before tpm_read() grabs the mutex.

Protect changes to chip->data_pending and chip->data_buffer by expanding
the scope of chip->buffer_mutex.

Reported-by: Seth Forshee <seth.forshee@canonical.com>
Cc: Debora Velarde <debora@linux.vnet.ibm.com>
Cc: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Cc: Marcel Selhorst <m.selhorst@sirrix.com>
Cc: tpmdd-devel@lists.sourceforge.net
Cc: stable@vger.kernel.org
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 drivers/char/tpm/tpm.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index b366b34..70bf9e5 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1074,12 +1074,15 @@ ssize_t tpm_write(struct file *file, const char __user *buf,
 	struct tpm_chip *chip = file->private_data;
 	size_t in_size = size, out_size;
 
+	mutex_lock(&chip->buffer_mutex);
+
 	/* cannot perform a write until the read has cleared
 	   either via tpm_read or a user_read_timer timeout */
-	while (atomic_read(&chip->data_pending) != 0)
+	while (atomic_read(&chip->data_pending) != 0) {
+		mutex_unlock(&chip->buffer_mutex);
 		msleep(TPM_TIMEOUT);
-
-	mutex_lock(&chip->buffer_mutex);
+		mutex_lock(&chip->buffer_mutex);
+	}
 
 	if (in_size > TPM_BUFSIZE)
 		in_size = TPM_BUFSIZE;
@@ -1112,22 +1115,20 @@ ssize_t tpm_read(struct file *file, char __user *buf,
 
 	del_singleshot_timer_sync(&chip->user_read_timer);
 	flush_work_sync(&chip->work);
-	ret_size = atomic_read(&chip->data_pending);
-	atomic_set(&chip->data_pending, 0);
+	mutex_lock(&chip->buffer_mutex);
+	ret_size = atomic_xchg(&chip->data_pending, 0);
 	if (ret_size > 0) {	/* relay data */
 		ssize_t orig_ret_size = ret_size;
 		if (size < ret_size)
 			ret_size = size;
 
-		mutex_lock(&chip->buffer_mutex);
 		rc = copy_to_user(buf, chip->data_buffer, ret_size);
 		memset(chip->data_buffer, 0, orig_ret_size);
 		if (rc)
 			ret_size = -EFAULT;
-
-		mutex_unlock(&chip->buffer_mutex);
 	}
 
+	mutex_unlock(&chip->buffer_mutex);
 	return ret_size;
 }
 EXPORT_SYMBOL_GPL(tpm_read);
-- 
1.7.0.4

[PATCH 3/3] TPM: data_pending is no longer atomic

[Tim Gardner]

Now that data_pending is fully protected by a mutex, it no longer
needs to be atomic.

Reported-by: Seth Forshee <seth.forshee@canonical.com>
Cc: Debora Velarde <debora@linux.vnet.ibm.com>
Cc: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Cc: Marcel Selhorst <m.selhorst@sirrix.com>
Cc: tpmdd-devel@lists.sourceforge.net
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 drivers/char/tpm/tpm.c |   16 ++++++++--------
 drivers/char/tpm/tpm.h |    2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 70bf9e5..6ac164f 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -342,7 +342,7 @@ static void timeout_work(struct work_struct *work)
 	struct tpm_chip *chip = container_of(work, struct tpm_chip, work);
 
 	mutex_lock(&chip->buffer_mutex);
-	atomic_set(&chip->data_pending, 0);
+	chip->data_pending = 0;
 	memset(chip->data_buffer, 0, TPM_BUFSIZE);
 	mutex_unlock(&chip->buffer_mutex);
 }
@@ -1043,7 +1043,7 @@ int tpm_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 	}
 
-	atomic_set(&chip->data_pending, 0);
+	chip->data_pending = 0;
 
 	file->private_data = chip;
 	return 0;
@@ -1060,7 +1060,7 @@ int tpm_release(struct inode *inode, struct file *file)
 	del_singleshot_timer_sync(&chip->user_read_timer);
 	flush_work_sync(&chip->work);
 	file->private_data = NULL;
-	atomic_set(&chip->data_pending, 0);
+	chip->data_pending = 0;
 	kfree(chip->data_buffer);
 	clear_bit(0, &chip->is_open);
 	put_device(chip->dev);
@@ -1078,7 +1078,7 @@ ssize_t tpm_write(struct file *file, const char __user *buf,
 
 	/* cannot perform a write until the read has cleared
 	   either via tpm_read or a user_read_timer timeout */
-	while (atomic_read(&chip->data_pending) != 0) {
+	while (chip->data_pending != 0) {
 		mutex_unlock(&chip->buffer_mutex);
 		msleep(TPM_TIMEOUT);
 		mutex_lock(&chip->buffer_mutex);
@@ -1096,7 +1096,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);
 
-	atomic_set(&chip->data_pending, out_size);
+	chip->data_pending = out_size;
 	mutex_unlock(&chip->buffer_mutex);
 
 	/* Set a timeout by which the reader must come claim the result */
@@ -1116,18 +1116,18 @@ ssize_t tpm_read(struct file *file, char __user *buf,
 	del_singleshot_timer_sync(&chip->user_read_timer);
 	flush_work_sync(&chip->work);
 	mutex_lock(&chip->buffer_mutex);
-	ret_size = atomic_xchg(&chip->data_pending, 0);
+	ret_size = chip->data_pending;
 	if (ret_size > 0) {	/* relay data */
-		ssize_t orig_ret_size = ret_size;
 		if (size < ret_size)
 			ret_size = size;
 
 		rc = copy_to_user(buf, chip->data_buffer, ret_size);
-		memset(chip->data_buffer, 0, orig_ret_size);
+		memset(chip->data_buffer, 0, chip->data_pending);
 		if (rc)
 			ret_size = -EFAULT;
 	}
 
+	chip->data_pending = 0;
 	mutex_unlock(&chip->buffer_mutex);
 	return ret_size;
 }
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 9c4163c..6ee8064 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -103,7 +103,7 @@ struct tpm_chip {
 
 	/* Data passed to and from the tpm via the read/write calls */
 	u8 *data_buffer;
-	atomic_t data_pending;
+	size_t data_pending;
 	struct mutex buffer_mutex;
 
 	struct timer_list user_read_timer;	/* user needs to claim result */
-- 
1.7.0.4

Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races

[Rajiv Andrade]

On 06/12/11 16:29, Tim Gardner wrote:
> There is a race betwen tpm_read() and tpm_write where both chip->data_pending
> and chip->data_buffer can be changed by tpm_write() when tpm_read()
> clears chip->data_pending, but before tpm_read() grabs the mutex.
>
> Protect changes to chip->data_pending and chip->data_buffer by expanding
> the scope of chip->buffer_mutex.
>
> Reported-by: Seth Forshee<seth.forshee@canonical.com>
> Cc: Debora Velarde<debora@linux.vnet.ibm.com>
> Cc: Rajiv Andrade<srajiv@linux.vnet.ibm.com>
> Cc: Marcel Selhorst<m.selhorst@sirrix.com>
> Cc: tpmdd-devel@lists.sourceforge.net
> Cc: stable@vger.kernel.org
> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
> ---
>   drivers/char/tpm/tpm.c |   17 +++++++++--------
>   1 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index b366b34..70bf9e5 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -1074,12 +1074,15 @@ ssize_t tpm_write(struct file *file, const char __user *buf,
>   	struct tpm_chip *chip = file->private_data;
>   	size_t in_size = size, out_size;
>
> +	mutex_lock(&chip->buffer_mutex);
> +
>   	/* cannot perform a write until the read has cleared
>   	   either via tpm_read or a user_read_timer timeout */
> -	while (atomic_read(&chip->data_pending) != 0)
> +	while (atomic_read(&chip->data_pending) != 0) {
> +		mutex_unlock(&chip->buffer_mutex);
>   		msleep(TPM_TIMEOUT);
> -
> -	mutex_lock(&chip->buffer_mutex);
> +		mutex_lock(&chip->buffer_mutex);
> +	}
>
>   	if (in_size>  TPM_BUFSIZE)
>   		in_size = TPM_BUFSIZE;
> @@ -1112,22 +1115,20 @@ ssize_t tpm_read(struct file *file, char __user *buf,
>
>   	del_singleshot_timer_sync(&chip->user_read_timer);
>   	flush_work_sync(&chip->work);
> -	ret_size = atomic_read(&chip->data_pending);
> -	atomic_set(&chip->data_pending, 0);
> +	mutex_lock(&chip->buffer_mutex);
> +	ret_size = atomic_xchg(&chip->data_pending, 0);
>   	if (ret_size>  0) {	/* relay data */
>   		ssize_t orig_ret_size = ret_size;
>   		if (size<  ret_size)
>   			ret_size = size;
>
> -		mutex_lock(&chip->buffer_mutex);
>   		rc = copy_to_user(buf, chip->data_buffer, ret_size);
>   		memset(chip->data_buffer, 0, orig_ret_size);
>   		if (rc)
>   			ret_size = -EFAULT;

What about just moving atomic_set(&chip->data_pending, 0); to here?
If I'm not missing anything, this would be cleaner.

Rajiv
> -
> -		mutex_unlock(&chip->buffer_mutex);
>   	}
>
> +	mutex_unlock(&chip->buffer_mutex);
>   	return ret_size;
>   }
>   EXPORT_SYMBOL_GPL(tpm_read);

Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races

[Tim Gardner]

On 12/20/2011 09:38 AM, Rajiv Andrade wrote:
> On 06/12/11 16:29, Tim Gardner wrote:
>> There is a race betwen tpm_read() and tpm_write where both
>> chip->data_pending
>> and chip->data_buffer can be changed by tpm_write() when tpm_read()
>> clears chip->data_pending, but before tpm_read() grabs the mutex.
>>
>> Protect changes to chip->data_pending and chip->data_buffer by expanding
>> the scope of chip->buffer_mutex.
>>
>> Reported-by: Seth Forshee<seth.forshee@canonical.com>
>> Cc: Debora Velarde<debora@linux.vnet.ibm.com>
>> Cc: Rajiv Andrade<srajiv@linux.vnet.ibm.com>
>> Cc: Marcel Selhorst<m.selhorst@sirrix.com>
>> Cc: tpmdd-devel@lists.sourceforge.net
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
>> ---
>> drivers/char/tpm/tpm.c | 17 +++++++++--------
>> 1 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
>> index b366b34..70bf9e5 100644
>> --- a/drivers/char/tpm/tpm.c
>> +++ b/drivers/char/tpm/tpm.c
>> @@ -1074,12 +1074,15 @@ ssize_t tpm_write(struct file *file, const
>> char __user *buf,
>> struct tpm_chip *chip = file->private_data;
>> size_t in_size = size, out_size;
>>
>> + mutex_lock(&chip->buffer_mutex);
>> +
>> /* cannot perform a write until the read has cleared
>> either via tpm_read or a user_read_timer timeout */
>> - while (atomic_read(&chip->data_pending) != 0)
>> + while (atomic_read(&chip->data_pending) != 0) {
>> + mutex_unlock(&chip->buffer_mutex);
>> msleep(TPM_TIMEOUT);
>> -
>> - mutex_lock(&chip->buffer_mutex);
>> + mutex_lock(&chip->buffer_mutex);
>> + }
>>
>> if (in_size> TPM_BUFSIZE)
>> in_size = TPM_BUFSIZE;
>> @@ -1112,22 +1115,20 @@ ssize_t tpm_read(struct file *file, char
>> __user *buf,
>>
>> del_singleshot_timer_sync(&chip->user_read_timer);
>> flush_work_sync(&chip->work);
>> - ret_size = atomic_read(&chip->data_pending);
>> - atomic_set(&chip->data_pending, 0);
>> + mutex_lock(&chip->buffer_mutex);
>> + ret_size = atomic_xchg(&chip->data_pending, 0);
>> if (ret_size> 0) { /* relay data */
>> ssize_t orig_ret_size = ret_size;
>> if (size< ret_size)
>> ret_size = size;
>>
>> - mutex_lock(&chip->buffer_mutex);
>> rc = copy_to_user(buf, chip->data_buffer, ret_size);
>> memset(chip->data_buffer, 0, orig_ret_size);
>> if (rc)
>> ret_size = -EFAULT;
>
> What about just moving atomic_set(&chip->data_pending, 0); to here?
> If I'm not missing anything, this would be cleaner.
>
> Rajiv

I'm not sure I agree. Moving just that statement doesn't close the race. 
Perhaps you could send me your version of this patch so that its clear 
what you are suggesting.

rtg
-- 
Tim Gardner tim.gardner@canonical.com

Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races

[Rajiv Andrade]

On 20-12-2011 17:39, Tim Gardner wrote:
> On 12/20/2011 09:38 AM, Rajiv Andrade wrote:
>> On 06/12/11 16:29, Tim Gardner wrote:
>>> There is a race betwen tpm_read() and tpm_write where both
>>> chip->data_pending
>>> and chip->data_buffer can be changed by tpm_write() when tpm_read()
>>> clears chip->data_pending, but before tpm_read() grabs the mutex.
>>>
>>> Protect changes to chip->data_pending and chip->data_buffer by 
>>> expanding
>>> the scope of chip->buffer_mutex.
>>>
>>> Reported-by: Seth Forshee<seth.forshee@canonical.com>
>>> Cc: Debora Velarde<debora@linux.vnet.ibm.com>
>>> Cc: Rajiv Andrade<srajiv@linux.vnet.ibm.com>
>>> Cc: Marcel Selhorst<m.selhorst@sirrix.com>
>>> Cc: tpmdd-devel@lists.sourceforge.net
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
>>> ---
>>> drivers/char/tpm/tpm.c | 17 +++++++++--------
>>> 1 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
>>> index b366b34..70bf9e5 100644
>>> --- a/drivers/char/tpm/tpm.c
>>> +++ b/drivers/char/tpm/tpm.c
>>> @@ -1074,12 +1074,15 @@ ssize_t tpm_write(struct file *file, const
>>> char __user *buf,
>>> struct tpm_chip *chip = file->private_data;
>>> size_t in_size = size, out_size;
>>>
>>> + mutex_lock(&chip->buffer_mutex);
>>> +
>>> /* cannot perform a write until the read has cleared
>>> either via tpm_read or a user_read_timer timeout */
>>> - while (atomic_read(&chip->data_pending) != 0)
>>> + while (atomic_read(&chip->data_pending) != 0) {
>>> + mutex_unlock(&chip->buffer_mutex);
>>> msleep(TPM_TIMEOUT);
>>> -
>>> - mutex_lock(&chip->buffer_mutex);
>>> + mutex_lock(&chip->buffer_mutex);
>>> + }
>>>
>>> if (in_size> TPM_BUFSIZE)
>>> in_size = TPM_BUFSIZE;
>>> @@ -1112,22 +1115,20 @@ ssize_t tpm_read(struct file *file, char
>>> __user *buf,
>>>
>>> del_singleshot_timer_sync(&chip->user_read_timer);
>>> flush_work_sync(&chip->work);
>>> - ret_size = atomic_read(&chip->data_pending);
>>> - atomic_set(&chip->data_pending, 0);
>>> + mutex_lock(&chip->buffer_mutex);
>>> + ret_size = atomic_xchg(&chip->data_pending, 0);
>>> if (ret_size> 0) { /* relay data */
>>> ssize_t orig_ret_size = ret_size;
>>> if (size< ret_size)
>>> ret_size = size;
>>>
>>> - mutex_lock(&chip->buffer_mutex);
>>> rc = copy_to_user(buf, chip->data_buffer, ret_size);
>>> memset(chip->data_buffer, 0, orig_ret_size);
>>> if (rc)
>>> ret_size = -EFAULT;
>>
>> What about just moving atomic_set(&chip->data_pending, 0); to here?
>> If I'm not missing anything, this would be cleaner.
>>
>> Rajiv
>
> I'm not sure I agree. Moving just that statement doesn't close the 
> race. Perhaps you could send me your version of this patch so that its 
> clear what you are suggesting.
>
> rtg
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 6a8771f..6a37212b 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1210,7 +1210,6 @@ ssize_t tpm_read(struct file *file, char __user *buf,
         del_singleshot_timer_sync(&chip->user_read_timer);
         flush_work_sync(&chip->work);
         ret_size = atomic_read(&chip->data_pending);
-       atomic_set(&chip->data_pending, 0);
         if (ret_size>  0) {     /* relay data */
                 if (size<  ret_size)
                         ret_size = size;
@@ -1223,6 +1222,7 @@ ssize_t tpm_read(struct file *file, char __user *buf,
                 mutex_unlock(&chip->buffer_mutex);
         }  
+       atomic_set(&chip->data_pending, 0);
         return ret_size;
  }

If we reset chip->data_pending after the buffer was copied to userspace,
it's guaranteed that tpm_write() won't touch such buffer before tpm_read()
handles it, given it polls chip->data_pending first.

Rajiv

Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races

[Tim Gardner]

On 12/22/2011 10:42 AM, Rajiv Andrade wrote:
> On 20-12-2011 17:39, Tim Gardner wrote:
>> On 12/20/2011 09:38 AM, Rajiv Andrade wrote:
>>> On 06/12/11 16:29, Tim Gardner wrote:
>>>> There is a race betwen tpm_read() and tpm_write where both
>>>> chip->data_pending
>>>> and chip->data_buffer can be changed by tpm_write() when tpm_read()
>>>> clears chip->data_pending, but before tpm_read() grabs the mutex.
>>>>
>>>> Protect changes to chip->data_pending and chip->data_buffer by
>>>> expanding
>>>> the scope of chip->buffer_mutex.
>>>>
>>>> Reported-by: Seth Forshee<seth.forshee@canonical.com>
>>>> Cc: Debora Velarde<debora@linux.vnet.ibm.com>
>>>> Cc: Rajiv Andrade<srajiv@linux.vnet.ibm.com>
>>>> Cc: Marcel Selhorst<m.selhorst@sirrix.com>
>>>> Cc: tpmdd-devel@lists.sourceforge.net
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
>>>> ---
>>>> drivers/char/tpm/tpm.c | 17 +++++++++--------
>>>> 1 files changed, 9 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
>>>> index b366b34..70bf9e5 100644
>>>> --- a/drivers/char/tpm/tpm.c
>>>> +++ b/drivers/char/tpm/tpm.c
>>>> @@ -1074,12 +1074,15 @@ ssize_t tpm_write(struct file *file, const
>>>> char __user *buf,
>>>> struct tpm_chip *chip = file->private_data;
>>>> size_t in_size = size, out_size;
>>>>
>>>> + mutex_lock(&chip->buffer_mutex);
>>>> +
>>>> /* cannot perform a write until the read has cleared
>>>> either via tpm_read or a user_read_timer timeout */
>>>> - while (atomic_read(&chip->data_pending) != 0)
>>>> + while (atomic_read(&chip->data_pending) != 0) {
>>>> + mutex_unlock(&chip->buffer_mutex);
>>>> msleep(TPM_TIMEOUT);
>>>> -
>>>> - mutex_lock(&chip->buffer_mutex);
>>>> + mutex_lock(&chip->buffer_mutex);
>>>> + }
>>>>
>>>> if (in_size> TPM_BUFSIZE)
>>>> in_size = TPM_BUFSIZE;
>>>> @@ -1112,22 +1115,20 @@ ssize_t tpm_read(struct file *file, char
>>>> __user *buf,
>>>>
>>>> del_singleshot_timer_sync(&chip->user_read_timer);
>>>> flush_work_sync(&chip->work);
>>>> - ret_size = atomic_read(&chip->data_pending);
>>>> - atomic_set(&chip->data_pending, 0);
>>>> + mutex_lock(&chip->buffer_mutex);
>>>> + ret_size = atomic_xchg(&chip->data_pending, 0);
>>>> if (ret_size> 0) { /* relay data */
>>>> ssize_t orig_ret_size = ret_size;
>>>> if (size< ret_size)
>>>> ret_size = size;
>>>>
>>>> - mutex_lock(&chip->buffer_mutex);
>>>> rc = copy_to_user(buf, chip->data_buffer, ret_size);
>>>> memset(chip->data_buffer, 0, orig_ret_size);
>>>> if (rc)
>>>> ret_size = -EFAULT;
>>>
>>> What about just moving atomic_set(&chip->data_pending, 0); to here?
>>> If I'm not missing anything, this would be cleaner.
>>>
>>> Rajiv
>>
>> I'm not sure I agree. Moving just that statement doesn't close the
>> race. Perhaps you could send me your version of this patch so that its
>> clear what you are suggesting.
>>
>> rtg
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 6a8771f..6a37212b 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -1210,7 +1210,6 @@ ssize_t tpm_read(struct file *file, char __user *buf,
> del_singleshot_timer_sync(&chip->user_read_timer);
> flush_work_sync(&chip->work);
> ret_size = atomic_read(&chip->data_pending);
> - atomic_set(&chip->data_pending, 0);
> if (ret_size> 0) { /* relay data */
> if (size< ret_size)
> ret_size = size;
> @@ -1223,6 +1222,7 @@ ssize_t tpm_read(struct file *file, char __user *buf,
> mutex_unlock(&chip->buffer_mutex);
> } + atomic_set(&chip->data_pending, 0);
> return ret_size;
> }
>
> If we reset chip->data_pending after the buffer was copied to userspace,
> it's guaranteed that tpm_write() won't touch such buffer before tpm_read()
> handles it, given it polls chip->data_pending first.
>

NAK - this patch makes it worse (if I'm reading your email client 
garbled patch correctly). Now it races with tpm_write() as well as 
timeout_work(). You cannot futz with chip->data_pending outside of the 
exclusion zones. Consider what will happen if a user process just loops 
doing reads. chip->data_pending gets cleared every time tpm_read() is 
called, regardless of what tpm_write() or timeout_work() are doing at 
the time.

tpm_read() / tpm_write() is a simple producer consumer model. Just use 
mutexes in an uncomplicated way. There is no need for data_pending to be 
atomic_t.

rtg
-- 
Tim Gardner tim.gardner@canonical.com

Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races

[Rajiv Andrade]

Thanks, Rajiv Andrade Security Development IBM Linux Technology Center

On 22-12-2011 16:44, Tim Gardner wrote:
> On 12/22/2011 10:42 AM, Rajiv Andrade wrote:
>> On 20-12-2011 17:39, Tim Gardner wrote:
>>> On 12/20/2011 09:38 AM, Rajiv Andrade wrote:
>>>> On 06/12/11 16:29, Tim Gardner wrote:
>>>>> There is a race betwen tpm_read() and tpm_write where both
>>>>> chip->data_pending
>>>>> and chip->data_buffer can be changed by tpm_write() when tpm_read()
>>>>> clears chip->data_pending, but before tpm_read() grabs the mutex.
>>>>>
>>>>> Protect changes to chip->data_pending and chip->data_buffer by
>>>>> expanding
>>>>> the scope of chip->buffer_mutex.
>>>>>
>>>>> Reported-by: Seth Forshee<seth.forshee@canonical.com>
>>>>> Cc: Debora Velarde<debora@linux.vnet.ibm.com>
>>>>> Cc: Rajiv Andrade<srajiv@linux.vnet.ibm.com>
>>>>> Cc: Marcel Selhorst<m.selhorst@sirrix.com>
>>>>> Cc: tpmdd-devel@lists.sourceforge.net
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
>>>>> ---
>>>>> drivers/char/tpm/tpm.c | 17 +++++++++--------
>>>>> 1 files changed, 9 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
>>>>> index b366b34..70bf9e5 100644
>>>>> --- a/drivers/char/tpm/tpm.c
>>>>> +++ b/drivers/char/tpm/tpm.c
>>>>> @@ -1074,12 +1074,15 @@ ssize_t tpm_write(struct file *file, const
>>>>> char __user *buf,
>>>>> struct tpm_chip *chip = file->private_data;
>>>>> size_t in_size = size, out_size;
>>>>>
>>>>> + mutex_lock(&chip->buffer_mutex);
>>>>> +
>>>>> /* cannot perform a write until the read has cleared
>>>>> either via tpm_read or a user_read_timer timeout */
>>>>> - while (atomic_read(&chip->data_pending) != 0)
>>>>> + while (atomic_read(&chip->data_pending) != 0) {
>>>>> + mutex_unlock(&chip->buffer_mutex);
>>>>> msleep(TPM_TIMEOUT);
>>>>> -
>>>>> - mutex_lock(&chip->buffer_mutex);
>>>>> + mutex_lock(&chip->buffer_mutex);
>>>>> + }
>>>>>
>>>>> if (in_size> TPM_BUFSIZE)
>>>>> in_size = TPM_BUFSIZE;
>>>>> @@ -1112,22 +1115,20 @@ ssize_t tpm_read(struct file *file, char
>>>>> __user *buf,
>>>>>
>>>>> del_singleshot_timer_sync(&chip->user_read_timer);
>>>>> flush_work_sync(&chip->work);
>>>>> - ret_size = atomic_read(&chip->data_pending);
>>>>> - atomic_set(&chip->data_pending, 0);
>>>>> + mutex_lock(&chip->buffer_mutex);
>>>>> + ret_size = atomic_xchg(&chip->data_pending, 0);
>>>>> if (ret_size> 0) { /* relay data */
>>>>> ssize_t orig_ret_size = ret_size;
>>>>> if (size< ret_size)
>>>>> ret_size = size;
>>>>>
>>>>> - mutex_lock(&chip->buffer_mutex);
>>>>> rc = copy_to_user(buf, chip->data_buffer, ret_size);
>>>>> memset(chip->data_buffer, 0, orig_ret_size);
>>>>> if (rc)
>>>>> ret_size = -EFAULT;
>>>>
>>>> What about just moving atomic_set(&chip->data_pending, 0); to here?
>>>> If I'm not missing anything, this would be cleaner.
>>>>
>>>> Rajiv
>>>
>>> I'm not sure I agree. Moving just that statement doesn't close the
>>> race. Perhaps you could send me your version of this patch so that its
>>> clear what you are suggesting.
>>>
>>> rtg
>> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
>> index 6a8771f..6a37212b 100644
>> --- a/drivers/char/tpm/tpm.c
>> +++ b/drivers/char/tpm/tpm.c
>> @@ -1210,7 +1210,6 @@ ssize_t tpm_read(struct file *file, char __user 
>> *buf,
>> del_singleshot_timer_sync(&chip->user_read_timer);
>> flush_work_sync(&chip->work);
>> ret_size = atomic_read(&chip->data_pending);
>> - atomic_set(&chip->data_pending, 0);
>> if (ret_size> 0) { /* relay data */
>> if (size< ret_size)
>> ret_size = size;
>> @@ -1223,6 +1222,7 @@ ssize_t tpm_read(struct file *file, char __user 
>> *buf,
>> mutex_unlock(&chip->buffer_mutex);
>> } + atomic_set(&chip->data_pending, 0);
>> return ret_size;
>> }
>>
>> If we reset chip->data_pending after the buffer was copied to userspace,
>> it's guaranteed that tpm_write() won't touch such buffer before 
>> tpm_read()
>> handles it, given it polls chip->data_pending first.
>>
>
> NAK - this patch makes it worse (if I'm reading your email client 
> garbled patch correctly). Now it races with tpm_write() as well as 
> timeout_work(). You cannot futz with chip->data_pending outside of the 
> exclusion zones. Consider what will happen if a user process just 
> loops doing reads. chip->data_pending gets cleared every time 
> tpm_read() is called, regardless of what tpm_write() or timeout_work() 
> are doing at the time.

Not sure how it's displaying for you, but your mail client is eating all 
whitespaces when sending. Look back here what I said:

http://marc.info/?l=tpmdd-devel&m=132439922903276&w=2

It's inside the mutex region.

This would require another fix though. tpm_write() doesn't check 
tpm_transmit return code (and it should).
In case it returns an error (< 0), chip->data_pending would remain the 
same forever with that change.

>
> tpm_read() / tpm_write() is a simple producer consumer model. Just use 
> mutexes in an uncomplicated way. There is no need for data_pending to 
> be atomic_t.
>
> rtg

That's a separate patch, 3/3, which is good once chip->data_pending is 
handled inside such regions.

Rajiv

Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races

[Tim Gardner]

On 12/22/2011 01:02 PM, Rajiv Andrade wrote:
>
> Thanks, Rajiv Andrade Security Development IBM Linux Technology Center
>
> On 22-12-2011 16:44, Tim Gardner wrote:
>> On 12/22/2011 10:42 AM, Rajiv Andrade wrote:
>>> On 20-12-2011 17:39, Tim Gardner wrote:
>>>> On 12/20/2011 09:38 AM, Rajiv Andrade wrote:
>>>>> On 06/12/11 16:29, Tim Gardner wrote:
>>>>>> There is a race betwen tpm_read() and tpm_write where both
>>>>>> chip->data_pending
>>>>>> and chip->data_buffer can be changed by tpm_write() when tpm_read()
>>>>>> clears chip->data_pending, but before tpm_read() grabs the mutex.
>>>>>>
>>>>>> Protect changes to chip->data_pending and chip->data_buffer by
>>>>>> expanding
>>>>>> the scope of chip->buffer_mutex.
>>>>>>
>>>>>> Reported-by: Seth Forshee<seth.forshee@canonical.com>
>>>>>> Cc: Debora Velarde<debora@linux.vnet.ibm.com>
>>>>>> Cc: Rajiv Andrade<srajiv@linux.vnet.ibm.com>
>>>>>> Cc: Marcel Selhorst<m.selhorst@sirrix.com>
>>>>>> Cc: tpmdd-devel@lists.sourceforge.net
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
>>>>>> ---
>>>>>> drivers/char/tpm/tpm.c | 17 +++++++++--------
>>>>>> 1 files changed, 9 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
>>>>>> index b366b34..70bf9e5 100644
>>>>>> --- a/drivers/char/tpm/tpm.c
>>>>>> +++ b/drivers/char/tpm/tpm.c
>>>>>> @@ -1074,12 +1074,15 @@ ssize_t tpm_write(struct file *file, const
>>>>>> char __user *buf,
>>>>>> struct tpm_chip *chip = file->private_data;
>>>>>> size_t in_size = size, out_size;
>>>>>>
>>>>>> + mutex_lock(&chip->buffer_mutex);
>>>>>> +
>>>>>> /* cannot perform a write until the read has cleared
>>>>>> either via tpm_read or a user_read_timer timeout */
>>>>>> - while (atomic_read(&chip->data_pending) != 0)
>>>>>> + while (atomic_read(&chip->data_pending) != 0) {
>>>>>> + mutex_unlock(&chip->buffer_mutex);
>>>>>> msleep(TPM_TIMEOUT);
>>>>>> -
>>>>>> - mutex_lock(&chip->buffer_mutex);
>>>>>> + mutex_lock(&chip->buffer_mutex);
>>>>>> + }
>>>>>>
>>>>>> if (in_size> TPM_BUFSIZE)
>>>>>> in_size = TPM_BUFSIZE;
>>>>>> @@ -1112,22 +1115,20 @@ ssize_t tpm_read(struct file *file, char
>>>>>> __user *buf,
>>>>>>
>>>>>> del_singleshot_timer_sync(&chip->user_read_timer);
>>>>>> flush_work_sync(&chip->work);
>>>>>> - ret_size = atomic_read(&chip->data_pending);
>>>>>> - atomic_set(&chip->data_pending, 0);
>>>>>> + mutex_lock(&chip->buffer_mutex);
>>>>>> + ret_size = atomic_xchg(&chip->data_pending, 0);
>>>>>> if (ret_size> 0) { /* relay data */
>>>>>> ssize_t orig_ret_size = ret_size;
>>>>>> if (size< ret_size)
>>>>>> ret_size = size;
>>>>>>
>>>>>> - mutex_lock(&chip->buffer_mutex);
>>>>>> rc = copy_to_user(buf, chip->data_buffer, ret_size);
>>>>>> memset(chip->data_buffer, 0, orig_ret_size);
>>>>>> if (rc)
>>>>>> ret_size = -EFAULT;
>>>>>
>>>>> What about just moving atomic_set(&chip->data_pending, 0); to here?
>>>>> If I'm not missing anything, this would be cleaner.
>>>>>
>>>>> Rajiv
>>>>
>>>> I'm not sure I agree. Moving just that statement doesn't close the
>>>> race. Perhaps you could send me your version of this patch so that its
>>>> clear what you are suggesting.
>>>>
>>>> rtg
>>> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
>>> index 6a8771f..6a37212b 100644
>>> --- a/drivers/char/tpm/tpm.c
>>> +++ b/drivers/char/tpm/tpm.c
>>> @@ -1210,7 +1210,6 @@ ssize_t tpm_read(struct file *file, char __user
>>> *buf,
>>> del_singleshot_timer_sync(&chip->user_read_timer);
>>> flush_work_sync(&chip->work);
>>> ret_size = atomic_read(&chip->data_pending);
>>> - atomic_set(&chip->data_pending, 0);
>>> if (ret_size> 0) { /* relay data */
>>> if (size< ret_size)
>>> ret_size = size;
>>> @@ -1223,6 +1222,7 @@ ssize_t tpm_read(struct file *file, char __user
>>> *buf,
>>> mutex_unlock(&chip->buffer_mutex);
>>> } + atomic_set(&chip->data_pending, 0);
>>> return ret_size;
>>> }
>>>
>>> If we reset chip->data_pending after the buffer was copied to userspace,
>>> it's guaranteed that tpm_write() won't touch such buffer before
>>> tpm_read()
>>> handles it, given it polls chip->data_pending first.
>>>
>>
>> NAK - this patch makes it worse (if I'm reading your email client
>> garbled patch correctly). Now it races with tpm_write() as well as
>> timeout_work(). You cannot futz with chip->data_pending outside of the
>> exclusion zones. Consider what will happen if a user process just
>> loops doing reads. chip->data_pending gets cleared every time
>> tpm_read() is called, regardless of what tpm_write() or timeout_work()
>> are doing at the time.
>
> Not sure how it's displaying for you, but your mail client is eating all
> whitespaces when sending. Look back here what I said:
>
> http://marc.info/?l=tpmdd-devel&m=132439922903276&w=2
>

You're right. I'm not sure whats going on with my Thunderbird client, 
but it appears to be a change in behavior. I've been using this specific 
instance since Ubuntu 10.04 was released.

> It's inside the mutex region.
>

Actually, the patch you sent (https://lkml.org/lkml/2011/12/22/257) is 
_outside_ the mutex area, but I got your drift.

Even clearing chip->data_pending _inside_ the mutex area, I'm not sure 
it closes all of the race windows. I think its still possible to race 
with mod_timer() and del_singleshot_timer_sync(). Therein lies my point, 
if the exclusion code is not _obviously_ correct for something this 
simple, then its probably not. I think my patch is the correct approach.

> This would require another fix though. tpm_write() doesn't check
> tpm_transmit return code (and it should).
> In case it returns an error (< 0), chip->data_pending would remain the
> same forever with that change.
>

This observation is also correct, but not relevant to the exclusion 
races. It deserves a separate patch.

<snip>
-- 
Tim Gardner tim.gardner@canonical.com

Re: [tpmdd-devel] [PATCH 2/3] TPM: Close data_pending and data_buffer races

[Mimi Zohar]

On Fri, 2011-12-23 at 07:25 -0700, Tim Gardner wrote:
> On 12/22/2011 01:02 PM, Rajiv Andrade wrote:

<snip>

> > It's inside the mutex region.
> >
> 
> Actually, the patch you sent (https://lkml.org/lkml/2011/12/22/257) is 
> _outside_ the mutex area, but I got your drift.

Yes, thanks for pointing it out in your original comments. I haven't
tested the following patch, but perhaps it will help clarify the updated
version, that I think Rajiv was describing ... 

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 6a8771f..7dafd95 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1210,7 +1210,6 @@ ssize_t tpm_read(struct file *file, char __user *buf,
 	del_singleshot_timer_sync(&chip->user_read_timer);
 	flush_work_sync(&chip->work);
 	ret_size = atomic_read(&chip->data_pending);
-	atomic_set(&chip->data_pending, 0);
 	if (ret_size > 0) {	/* relay data */
 		if (size < ret_size)
 			ret_size = size;
@@ -1221,8 +1220,10 @@ ssize_t tpm_read(struct file *file, char __user *buf,
 		if (rc)
 			ret_size = -EFAULT;
 
+		atomic_set(&chip->data_pending, 0);
 		mutex_unlock(&chip->buffer_mutex);
-	}
+	} else if (ret_size < 0)
+		atomic_set(&chip->data_pending, 0);
 
 	return ret_size;
 }

> Even clearing chip->data_pending _inside_ the mutex area, I'm not sure 
> it closes all of the race windows. I think its still possible to race 
> with mod_timer() and del_singleshot_timer_sync(). Therein lies my point, 
> if the exclusion code is not _obviously_ correct for something this 
> simple, then its probably not. I think my patch is the correct approach.

With the above patch, tpm_read() resets the data_pending flag only after
copying the data to userspace, resolving the original race condition
described.

> > This would require another fix though. tpm_write() doesn't check
> > tpm_transmit return code (and it should).
> > In case it returns an error (< 0), chip->data_pending would remain the
> > same forever with that change.
> >
> 
> This observation is also correct, but not relevant to the exclusion 
> races. It deserves a separate patch.

With the above patch, tpm_read() resets the data_pending flag only if
set, resolving the other race condition(s).

thanks,

Mimi

Re: [tpmdd-devel] [PATCH 2/3] TPM: Close data_pending and data_buffer races

[Rajiv Andrade]

On Tue, 27 Dec 2011, Mimi Zohar wrote:

> On Fri, 2011-12-23 at 07:25 -0700, Tim Gardner wrote:
> > On 12/22/2011 01:02 PM, Rajiv Andrade wrote:
> 
> <snip>
> 
> > > It's inside the mutex region.
> > >
> > 
> > Actually, the patch you sent (https://lkml.org/lkml/2011/12/22/257) is 
> > _outside_ the mutex area, but I got your drift.
> 
> Yes, thanks for pointing it out in your original comments. I haven't
> tested the following patch, but perhaps it will help clarify the updated
> version, that I think Rajiv was describing ... 
>

That's exactly it Mimi, thanks for writing down the patch.

> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 6a8771f..7dafd95 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -1210,7 +1210,6 @@ ssize_t tpm_read(struct file *file, char __user *buf,
>  	del_singleshot_timer_sync(&chip->user_read_timer);
>  	flush_work_sync(&chip->work);
>  	ret_size = atomic_read(&chip->data_pending);
> -	atomic_set(&chip->data_pending, 0);
>  	if (ret_size > 0) {	/* relay data */
>  		if (size < ret_size)
>  			ret_size = size;
> @@ -1221,8 +1220,10 @@ ssize_t tpm_read(struct file *file, char __user *buf,
>  		if (rc)
>  			ret_size = -EFAULT;
> 
> +		atomic_set(&chip->data_pending, 0);
>  		mutex_unlock(&chip->buffer_mutex);
> -	}
> +	} else if (ret_size < 0)
> +		atomic_set(&chip->data_pending, 0);
> 
>  	return ret_size;
>  }

Tim, do you still see a race with the changes posted above?

Rajiv

> 
> > Even clearing chip->data_pending _inside_ the mutex area, I'm not sure 
> > it closes all of the race windows. I think its still possible to race 
> > with mod_timer() and del_singleshot_timer_sync(). Therein lies my point, 
> > if the exclusion code is not _obviously_ correct for something this 
> > simple, then its probably not. I think my patch is the correct approach.
> 
> With the above patch, tpm_read() resets the data_pending flag only after
> copying the data to userspace, resolving the original race condition
> described.
> 
> > > This would require another fix though. tpm_write() doesn't check
> > > tpm_transmit return code (and it should).
> > > In case it returns an error (< 0), chip->data_pending would remain the
> > > same forever with that change.
> > >
> > 
> > This observation is also correct, but not relevant to the exclusion 
> > races. It deserves a separate patch.
> 
> With the above patch, tpm_read() resets the data_pending flag only if
> set, resolving the other race condition(s).
> 
> thanks,
> 
> Mimi
> 

Re: [PATCH 1/3] TPM: Zero buffer whole after copying to userspace

[Rajiv Andrade]

On Tue, 06 Dec 2011, Tim Gardner wrote:

> Commit 3321c07ae5068568cd61ac9f4ba749006a7185c9 correctly clears the TPM
> buffer if the user specified read length is >= the TPM buffer length. However,
> if the user specified read length is < the TPM buffer length, then part of the
> TPM buffer is left uncleared.
> 
> Reported-by: Seth Forshee <seth.forshee@canonical.com>
> Cc: Debora Velarde <debora@linux.vnet.ibm.com>
> Cc: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> Cc: Marcel Selhorst <m.selhorst@sirrix.com>
> Cc: tpmdd-devel@lists.sourceforge.net
> Cc: stable@vger.kernel.org
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

Applied to github.com:srajiv/tpm.git next

Thanks,
Rajiv

Re: [tpmdd-devel] [PATCH 2/3] TPM: Close data_pending and data_buffer races

[Kent Yoder]

Hi,

On Wed, Jan 11, 2012 at 05:43:31PM -0200, Rajiv Andrade wrote:
> On Tue, 27 Dec 2011, Mimi Zohar wrote:
> 
> > On Fri, 2011-12-23 at 07:25 -0700, Tim Gardner wrote:
> > > On 12/22/2011 01:02 PM, Rajiv Andrade wrote:
> > 
> > <snip>
> > 
> > > > It's inside the mutex region.
> > > >
> > > 
> > > Actually, the patch you sent (https://lkml.org/lkml/2011/12/22/257) is 
> > > _outside_ the mutex area, but I got your drift.
> > 
> > Yes, thanks for pointing it out in your original comments. I haven't
> > tested the following patch, but perhaps it will help clarify the updated
> > version, that I think Rajiv was describing ... 
> >
> 
> That's exactly it Mimi, thanks for writing down the patch.
> 
> > diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> > index 6a8771f..7dafd95 100644
> > --- a/drivers/char/tpm/tpm.c
> > +++ b/drivers/char/tpm/tpm.c
> > @@ -1210,7 +1210,6 @@ ssize_t tpm_read(struct file *file, char __user *buf,
> >  	del_singleshot_timer_sync(&chip->user_read_timer);
> >  	flush_work_sync(&chip->work);
> >  	ret_size = atomic_read(&chip->data_pending);
> > -	atomic_set(&chip->data_pending, 0);
> >  	if (ret_size > 0) {	/* relay data */
> >  		if (size < ret_size)
> >  			ret_size = size;
> > @@ -1221,8 +1220,10 @@ ssize_t tpm_read(struct file *file, char __user *buf,
> >  		if (rc)
> >  			ret_size = -EFAULT;
> > 
> > +		atomic_set(&chip->data_pending, 0);
> >  		mutex_unlock(&chip->buffer_mutex);
> > -	}
> > +	} else if (ret_size < 0)
> > +		atomic_set(&chip->data_pending, 0);
> > 
> >  	return ret_size;
> >  }
> 
> Tim, do you still see a race with the changes posted above?

After some testing, Mimi's patch works, but I don't believe there's any
reason to put the atomic_set() inside the lock of buffer_mutex. There's
no issue with the timer popping because del_singleshot_timer_sync()
ensures its not running before tpm_read proceeds.

WRT to zeroing the buffer though, it looks like there's a second
issue. The data buffer could be returned to the OS via kfree if release is
called before read. read can't be called after release is called, but at
that point the buffer is already returned to the OS. I'll update that
with a kzfree(), also in this patch:

Thanks,
Kent

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index e46e869..2901c3e 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1163,7 +1163,7 @@ int tpm_release(struct inode *inode, struct file *file)
 	flush_work_sync(&chip->work);
 	file->private_data = NULL;
 	atomic_set(&chip->data_pending, 0);
-	kfree(chip->data_buffer);
+	kzfree(chip->data_buffer);
 	clear_bit(0, &chip->is_open);
 	put_device(chip->dev);
 	return 0;
@@ -1215,7 +1215,6 @@ ssize_t tpm_read(struct file *file, char __user *buf,
 	del_singleshot_timer_sync(&chip->user_read_timer);
 	flush_work_sync(&chip->work);
 	ret_size = atomic_read(&chip->data_pending);
-	atomic_set(&chip->data_pending, 0);
 	if (ret_size > 0) {	/* relay data */
 		ssize_t orig_ret_size = ret_size;
 		if (size < ret_size)
@@ -1230,6 +1229,8 @@ ssize_t tpm_read(struct file *file, char __user *buf,
 		mutex_unlock(&chip->buffer_mutex);
 	}
 
+	atomic_set(&chip->data_pending, 0);
+
 	return ret_size;
 }
 EXPORT_SYMBOL_GPL(tpm_read);