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

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

sashiko-bot@kernel.org writes:

> 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);
>>  }


I think this problem already exists upstream. We should delay
device_add() until PCI/TSM registration succeeds. The kobject event
should also be emitted after device_add(). I will add the fix as a
separate patch.

-aneesh