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

[Jason Gunthorpe]

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

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

> [ ... 6 lines skipped ... ]
>  void tsm_unregister(struct tsm_dev *tsm_dev)
>  {
> +	/* Remove the class lookup first. */
> +	device_del(&tsm_dev->dev);
>  	if (tsm_dev->pci_ops)
>  		pci_tsm_unregister(tsm_dev);
> -	device_unregister(&tsm_dev->dev);
> +	put_device(&tsm_dev->dev);

This makes sense at least..

-- 
Jason