* [PATCH 0/3] TPM: CVE patch, close a race, atomic cleanup
@ 2011-12-06 18:29 Tim Gardner
2011-12-06 18:29 ` [PATCH 1/3] TPM: Zero buffer whole after copying to userspace Tim Gardner
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Tim Gardner @ 2011-12-06 18:29 UTC (permalink / raw)
To: linux-kernel; +Cc: Seth Forshee, 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(-)
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] TPM: Zero buffer whole after copying to userspace 2011-12-06 18:29 [PATCH 0/3] TPM: CVE patch, close a race, atomic cleanup Tim Gardner @ 2011-12-06 18:29 ` Tim Gardner 2012-02-03 17:39 ` Rajiv Andrade 2011-12-06 18:29 ` [PATCH 2/3] TPM: Close data_pending and data_buffer races Tim Gardner 2011-12-06 18:29 ` [PATCH 3/3] TPM: data_pending is no longer atomic Tim Gardner 2 siblings, 1 reply; 14+ messages in thread From: Tim Gardner @ 2011-12-06 18:29 UTC (permalink / raw) To: linux-kernel Cc: Seth Forshee, Tim Gardner, Debora Velarde, Rajiv Andrade, Marcel Selhorst, tpmdd-devel, stable 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] TPM: Zero buffer whole after copying to userspace 2011-12-06 18:29 ` [PATCH 1/3] TPM: Zero buffer whole after copying to userspace Tim Gardner @ 2012-02-03 17:39 ` Rajiv Andrade 0 siblings, 0 replies; 14+ messages in thread From: Rajiv Andrade @ 2012-02-03 17:39 UTC (permalink / raw) To: Tim Gardner Cc: linux-kernel, Seth Forshee, Debora Velarde, Marcel Selhorst, tpmdd-devel, stable 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] TPM: Close data_pending and data_buffer races 2011-12-06 18:29 [PATCH 0/3] TPM: CVE patch, close a race, atomic cleanup Tim Gardner 2011-12-06 18:29 ` [PATCH 1/3] TPM: Zero buffer whole after copying to userspace Tim Gardner @ 2011-12-06 18:29 ` Tim Gardner 2011-12-20 16:38 ` Rajiv Andrade 2011-12-06 18:29 ` [PATCH 3/3] TPM: data_pending is no longer atomic Tim Gardner 2 siblings, 1 reply; 14+ messages in thread From: Tim Gardner @ 2011-12-06 18:29 UTC (permalink / raw) To: linux-kernel Cc: Seth Forshee, Tim Gardner, Debora Velarde, Rajiv Andrade, Marcel Selhorst, tpmdd-devel, stable 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races 2011-12-06 18:29 ` [PATCH 2/3] TPM: Close data_pending and data_buffer races Tim Gardner @ 2011-12-20 16:38 ` Rajiv Andrade 2011-12-20 19:39 ` Tim Gardner 0 siblings, 1 reply; 14+ messages in thread From: Rajiv Andrade @ 2011-12-20 16:38 UTC (permalink / raw) To: Tim Gardner Cc: linux-kernel, Seth Forshee, Debora Velarde, Marcel Selhorst, tpmdd-devel, stable 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); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races 2011-12-20 16:38 ` Rajiv Andrade @ 2011-12-20 19:39 ` Tim Gardner 2011-12-22 17:42 ` Rajiv Andrade 0 siblings, 1 reply; 14+ messages in thread From: Tim Gardner @ 2011-12-20 19:39 UTC (permalink / raw) To: Rajiv Andrade Cc: linux-kernel, Seth Forshee, Debora Velarde, Marcel Selhorst, tpmdd-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races 2011-12-20 19:39 ` Tim Gardner @ 2011-12-22 17:42 ` Rajiv Andrade 2011-12-22 18:44 ` Tim Gardner 0 siblings, 1 reply; 14+ messages in thread From: Rajiv Andrade @ 2011-12-22 17:42 UTC (permalink / raw) To: Tim Gardner Cc: linux-kernel, Seth Forshee, Debora Velarde, Marcel Selhorst, tpmdd-devel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races 2011-12-22 17:42 ` Rajiv Andrade @ 2011-12-22 18:44 ` Tim Gardner 2011-12-22 20:02 ` Rajiv Andrade 0 siblings, 1 reply; 14+ messages in thread From: Tim Gardner @ 2011-12-22 18:44 UTC (permalink / raw) To: Rajiv Andrade Cc: linux-kernel, Seth Forshee, Debora Velarde, Marcel Selhorst, tpmdd-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races 2011-12-22 18:44 ` Tim Gardner @ 2011-12-22 20:02 ` Rajiv Andrade 2011-12-23 14:25 ` Tim Gardner 0 siblings, 1 reply; 14+ messages in thread From: Rajiv Andrade @ 2011-12-22 20:02 UTC (permalink / raw) To: Tim Gardner Cc: linux-kernel, Seth Forshee, Debora Velarde, Marcel Selhorst, tpmdd-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] TPM: Close data_pending and data_buffer races 2011-12-22 20:02 ` Rajiv Andrade @ 2011-12-23 14:25 ` Tim Gardner 2011-12-27 20:02 ` [tpmdd-devel] " Mimi Zohar 0 siblings, 1 reply; 14+ messages in thread From: Tim Gardner @ 2011-12-23 14:25 UTC (permalink / raw) To: Rajiv Andrade Cc: linux-kernel, Seth Forshee, Debora Velarde, Marcel Selhorst, tpmdd-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [tpmdd-devel] [PATCH 2/3] TPM: Close data_pending and data_buffer races 2011-12-23 14:25 ` Tim Gardner @ 2011-12-27 20:02 ` Mimi Zohar 2012-01-11 19:43 ` Rajiv Andrade 0 siblings, 1 reply; 14+ messages in thread From: Mimi Zohar @ 2011-12-27 20:02 UTC (permalink / raw) To: Tim Gardner Cc: Rajiv Andrade, Seth Forshee, tpmdd-devel, linux-kernel, Marcel Selhorst 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [tpmdd-devel] [PATCH 2/3] TPM: Close data_pending and data_buffer races 2011-12-27 20:02 ` [tpmdd-devel] " Mimi Zohar @ 2012-01-11 19:43 ` Rajiv Andrade 2012-07-25 17:36 ` Kent Yoder 0 siblings, 1 reply; 14+ messages in thread From: Rajiv Andrade @ 2012-01-11 19:43 UTC (permalink / raw) To: Mimi Zohar Cc: Tim Gardner, Seth Forshee, tpmdd-devel, linux-kernel, Marcel Selhorst 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [tpmdd-devel] [PATCH 2/3] TPM: Close data_pending and data_buffer races 2012-01-11 19:43 ` Rajiv Andrade @ 2012-07-25 17:36 ` Kent Yoder 0 siblings, 0 replies; 14+ messages in thread From: Kent Yoder @ 2012-07-25 17:36 UTC (permalink / raw) To: Rajiv Andrade Cc: Mimi Zohar, Tim Gardner, Seth Forshee, tpmdd-devel, linux-kernel, Marcel Selhorst 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); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] TPM: data_pending is no longer atomic 2011-12-06 18:29 [PATCH 0/3] TPM: CVE patch, close a race, atomic cleanup Tim Gardner 2011-12-06 18:29 ` [PATCH 1/3] TPM: Zero buffer whole after copying to userspace Tim Gardner 2011-12-06 18:29 ` [PATCH 2/3] TPM: Close data_pending and data_buffer races Tim Gardner @ 2011-12-06 18:29 ` Tim Gardner 2 siblings, 0 replies; 14+ messages in thread From: Tim Gardner @ 2011-12-06 18:29 UTC (permalink / raw) To: linux-kernel Cc: Seth Forshee, Tim Gardner, Debora Velarde, Rajiv Andrade, Marcel Selhorst, tpmdd-devel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-07-25 18:23 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-12-06 18:29 [PATCH 0/3] TPM: CVE patch, close a race, atomic cleanup Tim Gardner 2011-12-06 18:29 ` [PATCH 1/3] TPM: Zero buffer whole after copying to userspace Tim Gardner 2012-02-03 17:39 ` Rajiv Andrade 2011-12-06 18:29 ` [PATCH 2/3] TPM: Close data_pending and data_buffer races Tim Gardner 2011-12-20 16:38 ` Rajiv Andrade 2011-12-20 19:39 ` Tim Gardner 2011-12-22 17:42 ` Rajiv Andrade 2011-12-22 18:44 ` Tim Gardner 2011-12-22 20:02 ` Rajiv Andrade 2011-12-23 14:25 ` Tim Gardner 2011-12-27 20:02 ` [tpmdd-devel] " Mimi Zohar 2012-01-11 19:43 ` Rajiv Andrade 2012-07-25 17:36 ` Kent Yoder 2011-12-06 18:29 ` [PATCH 3/3] TPM: data_pending is no longer atomic Tim Gardner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.