linux-kernel - Re: [tpmdd-devel] [PATCH 2/3] TPM: Close data_pending and data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 25 Jul 2012 12:36:11 -0500
From:	Kent Yoder <key@...ux.vnet.ibm.com>
To:	Rajiv Andrade <srajiv@...ux.vnet.ibm.com>
Cc:	Mimi Zohar <zohar@...ux.vnet.ibm.com>,
	Tim Gardner <tim.gardner@...onical.com>,
	Seth Forshee <seth.forshee@...onical.com>,
	tpmdd-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
	Marcel Selhorst <m.selhorst@...rix.com>
Subject: Re: [tpmdd-devel] [PATCH 2/3] TPM: Close data_pending and
 data_buffer races

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

--
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/