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

[Kent Yoder <key@linux.vnet.ibm.com>]

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);