| Message ID | 152701036671.19968.17347263774570787595.stgit@tstruk-mobl1.jf.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 22, 2018 at 10:32:46AM -0700, Tadeusz Struk wrote: > There is a race condition in tpm_common_write function allowing two > threads on the same /dev/tpm<N>, or two different applications on > the same /dev/tpmrm<N> to overwrite eachother requests/responses. > > Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> > --- > drivers/char/tpm/tpm-dev-common.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) I didn't see any reasn for data_pending to be an atomic, ever use case is near the buffer_mutex, can you respin this patch to just drop that completely and only manipulate it within the lock? Jason
On 05/22/2018 01:07 PM, Jason Gunthorpe wrote: > I didn't see any reasn for data_pending to be an atomic, ever use case > is near the buffer_mutex, can you respin this patch to just drop that > completely and only manipulate it within the lock? Yes, will do. Thanks,
On Tue, May 22, 2018 at 10:32:46AM -0700, Tadeusz Struk wrote: > There is a race condition in tpm_common_write function allowing two > threads on the same /dev/tpm<N>, or two different applications on > the same /dev/tpmrm<N> to overwrite eachother requests/responses. > > Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> Ouch o_O Do you have a fixes tag for this one? Thank you. Reviewed-by: Jarkko Sakkien <jarkko.sakkinen@linux.intel.com> /Jarkko
On 05/23/2018 06:23 AM, Jarkko Sakkinen wrote: > Ouch o_O Do you have a fixes tag for this one? > This one is quite tricky. The original bug was introduced by abce9ac292e13 (tpm: Propagate error from tpm_transmit to fix a timeout hang) and the code back then was in drivers/char/tpm/tpm-interface.c file Then there were two other commits that moved the code around: afdba32e2a9ea (tpm: Pull everything related to /dev/tpmX into tpm-dev.c) which moved it from drivers/char/tpm/tpm-interface.c into drivers/char/tpm/tpm-dev.c and last one, ecb38e2f521b0 (tpm: split out tpm-dev.c into tpm-dev.c and tpm-common-dev.c) which moved it from drivers/char/tpm/tpm-dev.c into tpm-common-dev.c I have no idea how to tag it. Maybe we can use: Fixes: ecb38e2f521b0 ("tpm: split out tpm-dev.c into tpm-dev.c and tpm-common-dev.c") And then it probably needs to be back ported manually all the way back to abce9ac292e13. Thanks,
On Wed, May 23, 2018 at 10:57:07AM -0700, Tadeusz Struk wrote: > On 05/23/2018 06:23 AM, Jarkko Sakkinen wrote: > > Ouch o_O Do you have a fixes tag for this one? > > > > This one is quite tricky. > The original bug was introduced by abce9ac292e13 (tpm: Propagate error from tpm_transmit to fix a timeout hang) > and the code back then was in drivers/char/tpm/tpm-interface.c file No, it has been wrong since before git history started, so just use Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") All the other commits you listed are just moving the bad code around to new files. Jason
On Wed, May 23, 2018 at 10:57:07AM -0700, Tadeusz Struk wrote: > On 05/23/2018 06:23 AM, Jarkko Sakkinen wrote: > > Ouch o_O Do you have a fixes tag for this one? > > > > This one is quite tricky. > The original bug was introduced by abce9ac292e13 (tpm: Propagate error from tpm_transmit to fix a timeout hang) > and the code back then was in drivers/char/tpm/tpm-interface.c file > > Then there were two other commits that moved the code around: > afdba32e2a9ea (tpm: Pull everything related to /dev/tpmX into tpm-dev.c) > which moved it from drivers/char/tpm/tpm-interface.c into drivers/char/tpm/tpm-dev.c > > and last one, ecb38e2f521b0 (tpm: split out tpm-dev.c into tpm-dev.c and tpm-common-dev.c) > which moved it from drivers/char/tpm/tpm-dev.c into tpm-common-dev.c > > I have no idea how to tag it. Maybe we can use: > Fixes: ecb38e2f521b0 ("tpm: split out tpm-dev.c into tpm-dev.c and tpm-common-dev.c") > > And then it probably needs to be back ported manually all the way back to abce9ac292e13. > > Thanks, > -- > Tadeusz Thank you. I'll cc this to stable. /Jarkko
On Wed, May 23, 2018 at 01:41:15PM -0600, Jason Gunthorpe wrote: > On Wed, May 23, 2018 at 10:57:07AM -0700, Tadeusz Struk wrote: > > On 05/23/2018 06:23 AM, Jarkko Sakkinen wrote: > > > Ouch o_O Do you have a fixes tag for this one? > > > > > > > This one is quite tricky. > > The original bug was introduced by abce9ac292e13 (tpm: Propagate error from tpm_transmit to fix a timeout hang) > > and the code back then was in drivers/char/tpm/tpm-interface.c file > > No, it has been wrong since before git history started, so just use > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > All the other commits you listed are just moving the bad code around > to new files. > > Jason OK, thank you Jason. I'll use that. /Jarkko
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c index 230b99288024..1ce6f989f40a 100644 --- a/drivers/char/tpm/tpm-dev-common.c +++ b/drivers/char/tpm/tpm-dev-common.c @@ -91,17 +91,19 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf, size_t in_size = size; ssize_t out_size; + if (in_size > TPM_BUFSIZE) + return -E2BIG; + + mutex_lock(&priv->buffer_mutex); + /* Cannot perform a write until the read has cleared either via * tpm_read or a user_read_timer timeout. This also prevents split * buffered writes from blocking here. */ - if (atomic_read(&priv->data_pending) != 0) + if (atomic_read(&priv->data_pending) != 0) { + mutex_unlock(&priv->buffer_mutex); return -EBUSY; - - if (in_size > TPM_BUFSIZE) - return -E2BIG; - - mutex_lock(&priv->buffer_mutex); + } if (copy_from_user (priv->data_buffer, (void __user *) buf, in_size)) {
There is a race condition in tpm_common_write function allowing two threads on the same /dev/tpm<N>, or two different applications on the same /dev/tpmrm<N> to overwrite eachother requests/responses. Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> --- drivers/char/tpm/tpm-dev-common.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)