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

[Nayna <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>]

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 broadly
>>>>>
>>>>>      CPU0                           CPU1
>>>>>
>>>>> fops->open()
>>>>>                                  securityfs_remove()
>>>>> 				kref_put(chip)
>>>>> 				kfree(chip)
>>>>> kref_get(data->chip.kref)
>
>>> 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?
>
> Yes, I'm worried that securityfs_remove doesn not guarentee that
> all threads running open() have completed and that no new threads can
> start an open(). If that is guarenteed then we are fine once the
> get_device is added.
>
> There might be some tricky thing guaranteeing that but I haven't found
> it..
>
>>> 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?
>
> It appears debugfs choose to do that, so yes.
>
> I'm not sure what a driver is supposed to do. The problem is managing
> the lifetime of the 'data' (aka i_private) memory. We are using a kref
> for 'data' so we need to know when it is s that open is fenced
>
> synchronize with
>
>>> 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).
>
> IIRC belive remove comes from the device core, so it should be present
> for spi also...
>
> This solution might work.. Assuming the inode_lock is safe to get
> within open.
>
> tpm_bios_measurements_open(struct inode *inode)
> {
> 	struct tpm_chip *chip;
>
> 	inode_lock(inode);
> 	if (!inode->i_private) {
> 		inode_unlock(inode);
> 		return -ENODEV;
> 	}
> 	chip = inode->i_private;
> 	get_device(&chip->dev);
> 	inode_unlock(inode);
> }

>
> tpm_bios_log_teardown()
> {
> 	for (dentry *I ...) {
> 	    struct inode *inode = d_inode(I);
> 	    // securityfs_remove does not fence open, do it ourselves
> 	    inode_lock(inode);
> 	    inode->i_private = NULL;
> 	    inode_unlock(inode);

Are we trying to say that, once the teardown() is started, no more 
opening of files are allowed, even if they are visible ?

But if open() has happened first, and then teardown(), in that case 
private data is already passed to seq_open(). That means here the 
behaviour will still be same as existing.. that opened files continue to 
access the data, and teardown will not be waiting for opened files to be 
closed() ? Is this correct understanding of what we are trying to do ?

Thanks & Regards,
    - Nayna

> 	    securityfs_remove(I);
> 	}
> }
>
> Jason
>


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