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

[Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Mon, Oct 03, 2016 at 10:35:16AM -0600, 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 securityfs_remove.
> > > 
> > > Right, the above would not work because it's not linked to
> > > securityfs_remove by any means.
> > > 
>  
> > You have to provide something factors more concrete. Otherwise,
> > I'm inclined to accept the approach in Naynas patch. It's an
> > improvement.
> 
> I said I haven't checked the patch yet to see if the lifetime model
> for 'data' with securityfs is correct. Only that it matches the only
> other user of this feature..
> 
> I looked more carefully, and I still can't find the right sort of
> locking in securityfs_remove..
> 
> > > Are you trying to say that after securityfs_remove() there might be a
> > > "grace period" when user space could still see the files visible and
> > > open them?
> 
> Sort of, the typical race is broadly
> 
>     CPU0                           CPU1
> 
> fops->open()
>                                 securityfs_remove()
> 				kref_put(chip)
> 				kfree(chip)
> kref_get(data->chip.kref)

I see. So could this be reproduced by:

1. Open binary_measurements.
2. rmmod tpm_tis
3. Read contents of binary_measurements.

> This race should always be analyzed when working with user files.
> 
> We deal with this situation in the other user interface:
> - cdev uses 'chip->cdev.kobj.parent = &chip->dev.kobj;' and the cdev
>   core handles get/put of the chip at the proper time
> - sysfs uses kernfs_drain which guarentees nothing is running in any
>   callback before returning

Yeah, I get it. These securityfs files are nasty in a way compared to
sysfs attributes that they are not connected to the device hierachy.

Their life-cyce management will always be side-channel stuff, which is
not that nice to maintain.

Rather than finding a perfect solution in the code I think a better
angle would be find ways to test and reproduce possible races, which
you already started in your response.

Right now we basically don't have any good acceptance criteria to make
any changes to securityfs stuff. Yes, you can do the analysis (and
should) but human mind is weak sometimes :)

/Jarkko

> I suspect securityfs_remove is defective in this regard. Eg debugfs is
> built on the same libfs scheme as securityfs and it incorporates the
> mechanism around 'debugfs_use_file_start/etc' to provide sensible
> removal fencing.
> 
> I don't know if there is a simple fix, so mabye the best thing is to
> just leave it be with a comment saying it securityfs_remove probably
> races with fops->open(), and that should be fixed inside securityfs
> not tpm.
> 
> The file operations are also missing '.owner = THIS_MODULE' which is
> bad as well.
> 
> Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot