Date:	Fri, 23 Dec 2011 07:25:40 -0700
From:	Tim Gardner <tim.gardner@...onical.com>
To:	Rajiv Andrade <srajiv@...ux.vnet.ibm.com>
CC:	linux-kernel@...r.kernel.org,
	Seth Forshee <seth.forshee@...onical.com>,
	Debora Velarde <debora@...ux.vnet.ibm.com>,
	Marcel Selhorst <m.selhorst@...rix.com>,
	tpmdd-devel@...ts.sourceforge.net
Subject: Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races

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@...onical.com>
>>>>>> Cc: Debora Velarde<debora@...ux.vnet.ibm.com>
>>>>>> Cc: Rajiv Andrade<srajiv@...ux.vnet.ibm.com>
>>>>>> Cc: Marcel Selhorst<m.selhorst@...rix.com>
>>>>>> Cc: tpmdd-devel@...ts.sourceforge.net
>>>>>> Cc: stable@...r.kernel.org
>>>>>> Signed-off-by: Tim Gardner<tim.gardner@...onical.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@...onical.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists