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

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

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.

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

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

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

Jason

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