Messages in this thread Patch in this message | | | Subject | Re: [tpmdd-devel] [PATCH 2/3] TPM: Close data_pending and data_buffer races | From | Mimi Zohar <> | Date | Tue, 27 Dec 2011 15:02:44 -0500 |
| |
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
| |