Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On 10/14/2016 12:21 AM, Nayna wrote: > > > On 10/01/2016 05:31 PM, Jarkko Sakkinen wrote: >> On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote: >>> Currently, the securityfs pseudo files for obtaining the firmware >>> event log are created whether the event log properties exist or not.

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On 10/01/2016 05:31 PM, Jarkko Sakkinen wrote: > On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote: >> Currently, the securityfs pseudo files for obtaining the firmware >> event log are created whether the event log properties exist or not. >> This patch creates ascii and bios

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On 10/12/2016 01:45 AM, Jason Gunthorpe wrote: > On Wed, Oct 12, 2016 at 12:41:05AM +0530, Nayna wrote: > >> Yeah, I actually tried this today. >> And on call of securityfs_remove(), release() gets called for the >> opened > > Are you saying securityfs_remove somehow causes a synchronous call to

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Wed, Oct 12, 2016 at 12:41:05AM +0530, Nayna wrote: > Yeah, I actually tried this today. > And on call of securityfs_remove(), release() gets called for the > opened Are you saying securityfs_remove somehow causes a synchronous call to release? How does that come about? > There are actually

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Mon, Oct 10, 2016 at 09:43:05AM +0530, Nayna wrote: > > > On 10/10/2016 08:51 AM, Jason Gunthorpe wrote: > >On Mon, Oct 10, 2016 at 07:23:33AM +0530, Nayna wrote: > > > >>And we pass this as private data to i_node in tpm_bios_log_setup. > > > >>So, we are referring chip as

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On 10/10/2016 08:51 AM, Jason Gunthorpe wrote: > On Mon, Oct 10, 2016 at 07:23:33AM +0530, Nayna wrote: > >> And we pass this as private data to i_node in tpm_bios_log_setup. > >> So, we are referring chip as i_node->i_private->chip. > > That probably works, but you can't use the i_private =

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Mon, Oct 10, 2016 at 07:23:33AM +0530, Nayna wrote: > And we pass this as private data to i_node in tpm_bios_log_setup. > So, we are referring chip as i_node->i_private->chip. That probably works, but you can't use the i_private = NULL scheme I outlined with that. Jason

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Sun, Oct 09, 2016 at 09:47:08AM +0530, Nayna wrote: > >>+ const struct tpm_securityfs_data *sfs_data = > >>+ (const struct tpm_securityfs_data *)inode->i_private; > >>+ const struct seq_operations *seqops = sfs_data->seqops; > > > >You need a get_device(>dev) here, and the

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On 10/03/2016 10:44 PM, Jason Gunthorpe wrote: > On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote: >> @@ -323,34 +315,19 @@ static int tpm_bios_measurements_open(struct inode >> *inode, >> struct file *file) >> { >> int err; >> -

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On 10/07/2016 01:40 AM, Jason Gunthorpe wrote: > On Fri, Oct 07, 2016 at 01:26:45AM +0530, Nayna wrote: > >> - there is no kref increment during eventlog fops or seq_ops operations. >> - fops and seq ops are parsing over memory buffer. fops->open() returns >> after giving the memory buffer(log)

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Fri, Oct 07, 2016 at 01:41:29AM +0530, Nayna wrote: > Are we trying to say that, once the teardown() is started, no more opening > of files are allowed, even if they are visible ? Yes. > But if open() has happened first, and then teardown(), in that case private > data is already passed to

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On 10/04/2016 10:42 PM, Jason Gunthorpe wrote: > On Tue, Oct 04, 2016 at 08:26:51AM +0300, Jarkko Sakkinen wrote: >> On Mon, Oct 03, 2016 at 03:11:29PM -0600, Jason Gunthorpe wrote: >>> On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote: >>> > Sort of, the typical race is

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Fri, Oct 07, 2016 at 01:28:47AM +0530, Nayna wrote: > >fops->open() > > securityfs_remove() > > kref_put(chip) > > kfree(chip) > >kref_get(data->chip.kref) > > I didn't understand which kref_get() are we

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Fri, Oct 07, 2016 at 01:26:45AM +0530, Nayna wrote: > - there is no kref increment during eventlog fops or seq_ops operations. > - fops and seq ops are parsing over memory buffer. fops->open() returns > after giving the memory buffer(log) to seq->open(). And, seq ops on reading > of log memory

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On 10/03/2016 10:05 PM, Jason Gunthorpe wrote: > On Mon, Oct 03, 2016 at 03:35:23PM +0300, Jarkko Sakkinen wrote: > The scheme you suggested is also way off the mark for how fops works, fops->close has no relation to the needed duration for 'data', the duration is related to

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Tue, Oct 04, 2016 at 11:12:31AM -0600, Jason Gunthorpe wrote: > On Tue, Oct 04, 2016 at 08:26:51AM +0300, Jarkko Sakkinen wrote: > > On Mon, Oct 03, 2016 at 03:11:29PM -0600, Jason Gunthorpe wrote: > > > On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote: > > > > > > > > Sort of,

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Tue, Oct 04, 2016 at 08:26:51AM +0300, Jarkko Sakkinen wrote: > On Mon, Oct 03, 2016 at 03:11:29PM -0600, Jason Gunthorpe wrote: > > On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote: > > > > > > Sort of, the typical race is broadly > > > > > > > > CPU0

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Mon, Oct 03, 2016 at 03:11:29PM -0600, Jason Gunthorpe wrote: > On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote: > > > > Sort of, the typical race is broadly > > > > > > CPU0 CPU1 > > > > > > fops->open() > > >

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote: > > Sort of, the typical race is broadly > > > > CPU0 CPU1 > > > > fops->open() > > securityfs_remove() > > kref_put(chip) > >

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote: > @@ -323,34 +315,19 @@ static int tpm_bios_measurements_open(struct inode > *inode, > struct file *file) > { > int err; > - struct tpm_bios_log *log; > struct seq_file *seq; > -

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Mon, Oct 03, 2016 at 03:35:23PM +0300, Jarkko Sakkinen wrote: > > > The scheme you suggested is also way off the mark for how fops works, > > > fops->close has no relation to the needed duration for 'data', the > > > duration is related to securityfs_remove. > > > > Right, the above would not

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Mon, Oct 03, 2016 at 03:20:13PM +0300, Jarkko Sakkinen wrote: > On Sun, Oct 02, 2016 at 03:25:51PM -0600, Jason Gunthorpe wrote: > > On Sat, Oct 01, 2016 at 10:32:39PM +0300, Jarkko Sakkinen wrote: > > > > chip held for the duration of any fops. Can open() start after the > > > > kref is

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Sun, Oct 02, 2016 at 03:25:51PM -0600, Jason Gunthorpe wrote: > On Sat, Oct 01, 2016 at 10:32:39PM +0300, Jarkko Sakkinen wrote: > > > chip held for the duration of any fops. Can open() start after the > > > kref is dropped, etc. > > > > Why not make tpm_securityfs_data refcounted in order to

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Sat, Oct 01, 2016 at 10:32:39PM +0300, Jarkko Sakkinen wrote: > On Sat, Oct 01, 2016 at 10:54:36AM -0600, Jason Gunthorpe wrote: > > On Sat, Oct 01, 2016 at 03:01:25PM +0300, Jarkko Sakkinen wrote: > > > > > > + struct tpmfs_data bin_sfs_data; > > > > + struct tpmfs_data

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Sat, Oct 01, 2016 at 10:54:36AM -0600, Jason Gunthorpe wrote: > On Sat, Oct 01, 2016 at 03:01:25PM +0300, Jarkko Sakkinen wrote: > > > > + struct tpm_securityfs_data bin_sfs_data; > > > + struct tpm_securityfs_data ascii_sfs_data; > > > > I think this is otherwise right but the struct name is

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Sat, Oct 01, 2016 at 03:01:25PM +0300, Jarkko Sakkinen wrote: > > + struct tpm_securityfs_data bin_sfs_data; > > + struct tpm_securityfs_data ascii_sfs_data; > > I think this is otherwise right but the struct name is very clunky. > First of all it doesn't own the data and IMHO now it kind

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Sat, Oct 01, 2016 at 03:01:25PM +0300, Jarkko Sakkinen wrote: > On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote: > > Currently, the securityfs pseudo files for obtaining the firmware > > event log are created whether the event log properties exist or not. > > This patch creates ascii

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote: > Currently, the securityfs pseudo files for obtaining the firmware > event log are created whether the event log properties exist or not. > This patch creates ascii and bios measurements pseudo files > only if read_log() is successful.

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Fri, Sep 30, 2016 at 08:42:13PM -0600, Jason Gunthorpe wrote: > On Fri, Sep 30, 2016 at 10:45:38PM +0300, Jarkko Sakkinen wrote: > > > Ok, this is interesting. What kind of refcounting bugs are related > > to existing approach? > > IIRC it was because the log was being processed in an fops

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Fri, Sep 30, 2016 at 10:45:38PM +0300, Jarkko Sakkinen wrote: > Ok, this is interesting. What kind of refcounting bugs are related > to existing approach? IIRC it was because the log was being processed in an fops open() callback, which itself was not properly serialized against chip

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Fri, Sep 30, 2016 at 01:11:12PM -0600, Jason Gunthorpe wrote: > On Fri, Sep 30, 2016 at 09:57:43PM +0300, Jarkko Sakkinen wrote: > > On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote: > > > Currently, the securityfs pseudo files for obtaining the firmware > > > event log are created

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Fri, Sep 30, 2016 at 09:57:43PM +0300, Jarkko Sakkinen wrote: > On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote: > > Currently, the securityfs pseudo files for obtaining the firmware > > event log are created whether the event log properties exist or not. > > This patch creates ascii

Re: [tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote: > Currently, the securityfs pseudo files for obtaining the firmware > event log are created whether the event log properties exist or not. > This patch creates ascii and bios measurements pseudo files > only if read_log() is successful. >

[tpmdd-devel] [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

Currently, the securityfs pseudo files for obtaining the firmware event log are created whether the event log properties exist or not. This patch creates ascii and bios measurements pseudo files only if read_log() is successful. Suggested-by: Jason Gunthorpe