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 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()
> > >                                 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.
> 
> No, but that method shows the bug I pointed out in my email to Nayna
> where the fops stuff is not getting a kref on the chip.
> 
> You need to actually race open and securityfs_remove to see the
> kref_get() loose its race and then use-after-free.

So you are worried that get_device() might come when the chip is already
gone?

> > 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.
> 
> Well, it is not so bad, it is just missing the fence on removal that
> sysfs has, or the kref tracking that cdev has. Sadly this is a typical
> error within the fops stuff, I've seen it in many places.

Do you think that this should be fixed above the driver i.e. add fencing
to the securityfs code itself?

> > 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.
> 
> That would be very hard to do, racing two calls like that is
> quite difficult in any sort of automatic way, AFAIK.

I wonder if SPI has similar file to 'remove' that PCI devices have
(checking from the documentation later on).

> > 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 :)
> 
> Since it is unlikely and not caused by our subsystem I'm inclined to
> just leave a comment (that we expect securityfs_remove to fence) and
> you can send a note to James to see what they think.

Agreed.

> Jason

/Jarkko

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