Re: [PATCH v7 14/16] iommufd/viommu: Allow PCI TSM backends to provide vIOMMU operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci/tsm: Use-After-Free of tsm_dev due to missing device reference count
--

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

iommufd/viommu: Allow PCI TSM backends to provide vIOMMU operations

This commit allows a vIOMMU implementation to be provided by a TEE Security
Manager (TSM) rather than the physical IOMMU driver. It queries the TSM
associated with the target PCI device and resolves the PCI TSM association
under pci_tsm_rwsem while pinning the module.

> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> index dca9634f8d5eb..c8603867e09df 100644
> --- a/drivers/pci/tsm.c
> +++ b/drivers/pci/tsm.c

[ ... ]

> @@ -36,6 +38,25 @@ static const struct pci_tsm_ops *to_pci_tsm_ops(struct pci_tsm *tsm)
>  	return tsm->tsm_dev->pci_ops;
>  }
>  
> +struct tsm_dev *pci_tsm_get_device(struct pci_dev *pdev)
> +{
> +	const struct pci_tsm_ops *ops;
> +	struct tsm_dev *tsm_dev;
> +
> +	guard(rwsem_read)(&pci_tsm_rwsem);
> +	if (!pdev->tsm)
> +		return NULL;
> +
> +	tsm_dev = pdev->tsm->tsm_dev;
> +	ops = tsm_dev->pci_ops;
> +	if (!ops->viommu_get_ops)
> +		return NULL;
> +	if (!try_module_get(ops->owner))
> +		return ERR_PTR(-ENODEV);
> +	return tsm_dev;
> +}

[Severity: High]
Does this code need to take a kobject reference on the device via
get_device(&tsm_dev->dev)?

The tsm_dev pointer is returned and stored in viommu->tsm_dev for the
lifetime of the vIOMMU, relying solely on the module pin. 

The doc comment for tsm_get_device() states that backends must not unregister
their TSM independently of module unload. However, in the Linux device model,
a module pin only prevents module removal; it does not prevent a PCI device
from being unbound via sysfs (unbind or remove) or hardware hot-unplug.

If the device is unbound, the driver core forces the driver remove callback
to run, which calls tsm_unregister().

> diff --git a/drivers/virt/coco/tsm-core.c b/drivers/virt/coco/tsm-core.c
> index f791359861020..2fce341ff9231 100644
> --- a/drivers/virt/coco/tsm-core.c
> +++ b/drivers/virt/coco/tsm-core.c

[ ... ]

> @@ -16,6 +26,29 @@ static const struct class tsm_class = {
>  };
>  static DEFINE_IDA(tsm_ida);
>  
> +/**
> + * tsm_get_device() - Pin the TSM attached to a device
> + * @dev: device whose TSM is to be pinned
> + *
> + * A successful lookup pins the backend module. Backends providing vIOMMU
> + * operations must not unregister their TSM independently of module unload.
> + * Return: NULL if no vIOMMU-capable TSM is attached, an error if the module
> + * cannot be pinned, or the TSM device associated with the pinned backend.
> + */
> +struct tsm_dev *tsm_get_device(struct device *dev)
> +{
> +	if (!dev_is_pci(dev))
> +		return NULL;
> +	return pci_tsm_get_device(to_pci_dev(dev));
> +}
> +EXPORT_SYMBOL_GPL(tsm_get_device);
> +
> +void tsm_put_device(struct tsm_dev *tsm_dev)
> +{
> +	module_put(tsm_dev->pci_ops->owner);
> +}

[Severity: High]
Can this dereference freed memory?

When tsm_unregister() is called during an unbind or hot-unplug event, it
invokes put_device(&tsm_dev->dev). Because no extra reference was held when
acquiring the device in pci_tsm_get_device(), the reference count drops to zero
and the tsm_dev object is freed via kfree().

Later, when the vIOMMU is destroyed, iommufd_viommu_destroy() calls
tsm_put_device(viommu->tsm_dev). This dereferences the freed memory to access
tsm_dev->pci_ops->owner, resulting in a use-after-free crash.

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