Re: [PATCH v8 15/15] PCI/TSM: Wait for users during unregister

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI/TSM: use-after-free in tsm_unregister() and sev_tsm_uninit()
- [Low] PCI/TSM: mixing guard() and goto in pci_tsm_context_get() and pci_tsm_init()
--

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

PCI/TSM: Wait for users during unregister

This commit adds registration_lock and a user reference count to struct
tsm_dev so that tsm_unregister() and pci_tsm_destroy() wait for active TSM
users, including IOMMUFD virtual IOMMUs without vDEVICE objects, before
tearing down PCI connections.

> diff --git a/drivers/crypto/ccp/sev-dev-tsm.c b/drivers/crypto/ccp/sev-dev-tsm.c
> index 46f2539d2d5ab..5c61ce74ee0f7 100644
> --- a/drivers/crypto/ccp/sev-dev-tsm.c
> +++ b/drivers/crypto/ccp/sev-dev-tsm.c
> @@ -392,8 +392,8 @@ void sev_tsm_init_locked(struct sev_device *sev, void *tio_status_page)
>  
>  void sev_tsm_uninit(struct sev_device *sev)
>  {
> -	if (sev->tsmdev)
> -		tsm_unregister(sev->tsmdev);
> +	if (sev->tsmdev && WARN_ON(tsm_unregister(sev->tsmdev)))
> +		return;
>  
>  	sev->tsmdev = NULL;
>  }

[Severity: High]
Can a concurrent call to sev_tsm_uninit() while tsm_unregister() is waiting
for TSM users trigger a use-after-free or premature teardown?

In drivers/crypto/ccp/sev-dev.c, sev_firmware_shutdown() invokes
sev_tsm_uninit() outside sev_cmd_mutex:

drivers/crypto/ccp/sev-dev.c:sev_firmware_shutdown() {
	if (sev->tio_status)
		sev_tsm_uninit(sev);

	sev_snp_unregister_verify_mitigation(sev);

	mutex_lock(&sev_cmd_mutex);

	__sev_firmware_shutdown(sev, false);

	kfree(sev->tio_status);
	sev->tio_status = NULL;

	mutex_unlock(&sev_cmd_mutex);
}

Because tsm_unregister() now sleeps in tsm_wait_for_users() and
sev->tsmdev is only cleared after tsm_unregister() returns, a concurrent
caller of sev_firmware_shutdown() (such as sev_platform_shutdown() racing
with sev_dev_destroy() or sev_pci_exit()) can see non-NULL sev->tio_status
and sev->tsmdev and call tsm_unregister(sev->tsmdev) a second time.

Additionally, if tsm_unregister() returns -ENODEV here, sev_tsm_uninit()
returns early without clearing sev->tsmdev, and sev_firmware_shutdown()
proceeds to run __sev_firmware_shutdown() and free sev->tio_status while
the first caller is still blocked in tsm_wait_for_users() or disconnecting
PCI devices in pci_tsm_unregister().

> diff --git a/drivers/pci/tsm/core.c b/drivers/pci/tsm/core.c
> index 2c77321fa7792..49256638c164c 100644
> --- a/drivers/pci/tsm/core.c
> +++ b/drivers/pci/tsm/core.c

[ ... ]

> @@ -343,24 +352,33 @@ struct pci_tsm_context *pci_tsm_context_get(struct pci_dev *pdev,
>  		return ERR_PTR(-EOPNOTSUPP);
>  	/*
>  	 * The vdevice can use a different PCI function from VIOMMU_ALLOC. The
> -	 * module pin keeps the vIOMMU's TSM registered, but does not freeze this
> +	 * caller must retain its tsm_get_device() reference until the context is
> +	 * released. This keeps the TSM registered, but does not freeze this
>  	 * function's link. Match its current TSM before pinning the link below.
>  	 */
>  	if (pdev->tsm->tsm_dev != viommu_tsm_dev)
>  		return ERR_PTR(-EXDEV);
>  
> +	if (!tsm_trylock_registration(viommu_tsm_dev))
> +		return ERR_PTR(-ENODEV);
> +
>  	pf0 = to_pci_tsm_pf0(pdev->tsm);
> -	if (!pf0)
> -		return ERR_PTR(-ENXIO);
> +	if (!pf0) {
> +		context = ERR_PTR(-ENXIO);
> +		goto out_unlock_registration;
> +	}
>  
>  	context = kzalloc_obj(*context);
> -	if (!context)
> -		return ERR_PTR(-ENOMEM);
> +	if (!context) {
> +		context = ERR_PTR(-ENOMEM);
> +		goto out_unlock_registration;
> +	}
>  
> -	guard(mutex)(&pf0->lock);
> +	mutex_lock(&pf0->lock);
>  	if (pf0->removing) {
>  		kfree(context);
> -		return ERR_PTR(-ENODEV);
> +		context = ERR_PTR(-ENODEV);
> +		goto out_unlock_mutex;
>  	}
>  
>  	if (!pf0->dsm_context_users)
> @@ -370,6 +388,10 @@ struct pci_tsm_context *pci_tsm_context_get(struct pci_dev *pdev,
>  	context->pf0 = pf0;
>  	context->pdev = pci_dev_get(pdev);
>  	context->dsm_dev = pci_dev_get(pf0->base_tsm.pdev);
> +out_unlock_mutex:
> +	mutex_unlock(&pf0->lock);
> +out_unlock_registration:
> +	tsm_unlock_registration(viommu_tsm_dev);
>  	return context;
>  }
>  EXPORT_SYMBOL_GPL(pci_tsm_context_get);

[ ... ]

> @@ -842,11 +901,17 @@ void pci_tsm_init(struct pci_dev *pdev)
>  		if (!dsm->tsm)
>  			return;
>  
> +		tsm_dev = dsm->tsm->tsm_dev;
> +		if (!tsm_trylock_registration(tsm_dev))
> +			return;
> +
>  		if (is_link_tsm(dsm->tsm->tsm_dev) &&
>  		    to_pci_tsm_pf0(dsm->tsm)->removing)
> -			return;
> +			goto out_unlock_registration;
>  
>  		probe_fn(pdev, dsm);
> +out_unlock_registration:
> +		tsm_unlock_registration(tsm_dev);
>  	}
>  }

[Severity: Low]
This isn't a bug, but pci_tsm_context_get() and pci_tsm_init() both use
guard(rwsem_read)(&pci_tsm_rwsem) and now mix scope-based cleanup with
goto-based error handling (and pci_tsm_context_get() converts
guard(mutex)(&pf0->lock) back to manual mutex_lock() / mutex_unlock() with
goto labels).

Could these functions avoid mixing guard() with goto cleanup labels?

> diff --git a/drivers/virt/coco/tsm-core.c b/drivers/virt/coco/tsm-core.c
> index 92ce3337f1ea7..6bb373bc8bae0 100644
> --- a/drivers/virt/coco/tsm-core.c
> +++ b/drivers/virt/coco/tsm-core.c

[ ... ]

> +/**
> + * tsm_unregister() - unregister a TSM after draining its users
> + * @tsm_dev: TSM to unregister
> + *
> + * Blocks new users and waits for references acquired by tsm_get_device(),
> + * including vIOMMUs without vdevices, and pending DSM removals before tearing
> + * down PCI connections. The wait cannot be aborted by a signal; userspace
> + * must release the vIOMMUs and vdevices to let it finish.
> + *
> + * Return: 0 on success, or -ENODEV if unregistration has already started.
> + */
> +int tsm_unregister(struct tsm_dev *tsm_dev)
>  {
> -	/* Remove the class lookup first. */
> +	down_write(&tsm_dev->registration_lock);
> +	if (tsm_dev->unregistering) {
> +		up_write(&tsm_dev->registration_lock);
> +		return -ENODEV;
> +	}
> +	tsm_dev->unregistering = true;
> +
>  	device_del(&tsm_dev->dev);
> -	if (tsm_dev->pci_ops)
> +	/* No new user references can be acquired while the write lock is held. */
> +	tsm_put_user(tsm_dev);
> +	tsm_wait_for_users(tsm_dev);
> +
> +	if (IS_ENABLED(CONFIG_PCI_TSM) && tsm_dev->pci_ops)
>  		pci_tsm_unregister(tsm_dev);
> +
> +	up_write(&tsm_dev->registration_lock);
>  	put_device(&tsm_dev->dev);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tsm_unregister);

[Severity: High]
Can the tsm_dev->unregistering check in tsm_unregister() cause a
use-after-free on tsm_dev when two callers race to unregister the same
device?

tsm_register() initializes tsm_dev->dev with a single registration device
reference. When a first caller enters tsm_unregister(), it holds
down_write(&tsm_dev->registration_lock) across device_del(),
tsm_wait_for_users(), and pci_tsm_unregister().

If a second caller enters tsm_unregister() while the first caller is
waiting for users to drain, the second caller blocks in
down_write(&tsm_dev->registration_lock) without holding a device reference
on tsm_dev->dev. Once all user references have been dropped and the first
caller finishes:

	up_write(&tsm_dev->registration_lock);
	put_device(&tsm_dev->dev);

put_device(&tsm_dev->dev) drops the final reference and frees tsm_dev via
tsm_release(), while the second caller wakes from
down_write(&tsm_dev->registration_lock), reads tsm_dev->unregistering, and
calls up_write(&tsm_dev->registration_lock) on the freed tsm_dev.

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