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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tsm: race in tsm_register() error path leads to UAF and NULL pointer deref
--

commit f9b77b7469552ae6078c3a47333908a88eec2474
Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

tsm: Remove the device from lookup before PCI teardown

This commit modifies the teardown flow to remove the TSM device from
lookup before tearing down the PCI state. It also adjusts the setup
flow in tsm_register() to handle PCI registration failures differently.

> diff --git a/drivers/virt/coco/tsm-core.c b/drivers/virt/coco/tsm-core.c
> --- a/drivers/virt/coco/tsm-core.c
> +++ b/drivers/virt/coco/tsm-core.c
> @@ -56,26 +56,25 @@ static struct tsm_dev *alloc_tsm_dev(struct device *parent)
[ ... ]
> -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)
>  {
[ ... ]
>  	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?

If pci_tsm_register() returns an error early (e.g. if a driver incorrectly 
provides both link_ops and devsec_ops, returning -EINVAL before acquiring 
pci_tsm_rwsem), a concurrent userspace write to the connect sysfs attribute 
might trigger connect_store().

If connect_store() evaluates is_link_tsm() concurrently, this lockless 
assignment to NULL on the error path could cause the compiler to reload the 
pointer after the NULL check. Could this lead to a NULL pointer dereference 
when accessing ->link_ops.probe?

>  		dev_err(tsm_dev->dev.parent,
>  			"PCI/TSM registration failure: %d\n", rc);
> -		device_unregister(&tsm_dev->dev);
> -		return ERR_PTR(rc);
> +		return rc;
>  	}
[ ... ]
> @@ -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.

When this error path calls device_del() and returns, the __free(put_tsm_dev) 
guard at the top of tsm_register() will drop the primary reference to 
tsm_dev, freeing its memory.

Because the attached PCI context stores the pointer without taking an 
additional device reference, wouldn't this leave a dangling pointer in the 
PCI context, resulting in a use-after-free on subsequent accesses?

> +		return ERR_PTR(rc);
> +	}
> +	return no_free_ptr(tsm_dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008055955.4014342-1-aneesh.kumar@kernel.org?part=9