Re: [PATCH v7 09/16] tsm: Remove the device from lookup before PCI teardown

[Aneesh Kumar K . V]

Jason Gunthorpe <jgg@xxxxxxxxxx> writes:

>> [ ... 22 lines skipped ... ]
>> -static struct tsm_dev *tsm_register_pci_or_reset(struct tsm_dev *tsm_dev,
>> -						 struct pci_tsm_ops *pci_ops)
>> +static int tsm_register_pci(struct tsm_dev *tsm_dev, struct pci_tsm_ops *pci_ops)
>>  {
>>  	int rc;
>>  
>>  	if (!pci_ops)
>> -		return tsm_dev;
>> +		return 0;
>
> Aside unrelated to this series: Do we need this NULL pci_ops stuff? I
> don't know what it was imagined to do, but now that we have split
> things up attestation subsystem I wonder if it would ever be useful?
>
> It is quite a maze of unused layers in here..
>


Agreed. I'll remove it; we can always add it back if a use case arises.

>
>>  
>>  	tsm_dev->pci_ops = pci_ops;
>>  	rc = pci_tsm_register(tsm_dev);
>>  	if (rc) {
>> +		tsm_dev->pci_ops = NULL;
>  [Severity: Medium]
>  Does this unprotected write to tsm_dev->pci_ops introduce a race condition?
>
> And of course sashiko finds races in all this unused complexity...
>
>> [ ... 14 lines skipped ... ]
>> @@ -96,15 +95,22 @@ struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops)
>>  	if (rc)
>>  		return ERR_PTR(rc);
>>  
>> -	return tsm_register_pci_or_reset(no_free_ptr(tsm_dev), pci_ops);
>> +	rc = tsm_register_pci(tsm_dev, pci_ops);
>> +	if (rc) {
>> +		device_del(dev);
>  [Severity: Medium]
>  Can this error path lead to a use-after-free if PCI registration fails?
>
>  Since device_add() is called prior to tsm_register_pci() in tsm_register(), 
>  the device is already exposed via class_find_device(). If a concurrent 
>  connect_store() finds the device and proceeds to pci_tsm_connect() before 
>  this error path cleans it up, it will attach the PCI context to tsm_dev.
>
> Fun. Usually device_add should be the last thing in any driver flow,
> not the first.  There should not be any device_del on error paths.
>
> Because of races like this...
>

I now have:

struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops)
{
	struct tsm_dev *tsm_dev __free(put_tsm_dev) = alloc_tsm_dev(parent);
	struct device *dev;
	int rc;

	if (IS_ERR(tsm_dev))
		return tsm_dev;

	dev = &tsm_dev->dev;
	rc = dev_set_name(dev, "tsm%d", tsm_dev->id);
	if (rc)
		return ERR_PTR(rc);

	/* Complete PCI registration before exposing the TSM to class lookup. */
	rc = tsm_register_pci(tsm_dev, pci_ops);
	if (rc)
		return ERR_PTR(rc);

	rc = device_add(dev);
	if (rc) {
		if (pci_ops)
			pci_tsm_unregister(tsm_dev);
		return ERR_PTR(rc);
	}

	/* Notify TSM userspace that PCI/TSM operations are now possible. */
	if (pci_ops)
		kobject_uevent(&dev->kobj, KOBJ_CHANGE);

	return no_free_ptr(tsm_dev);
}

-aneesh