Re: [PATCH 10/15] PCI/TSM: Add device interface security LOCKED support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Sysfs operations `unlock_store` and `disconnect_store` do not verify the active TSM context type (`pdev->tsm`), allowing userspace to trigger kernel panics by performing operations on the wrong context type.
- [High] `pci_tsm_unlock()` returns early without clearing the TSM context if a driver is attached, causing a dangling pointer and Use-After-Free during subsystem teardown.

Pre-existing issues:
- [High] A TOCTOU race condition in `tsm_unregister()` allows `lock_store()` to attach a PCI device to a TSM that is actively being destroyed.
--

commit 82cae08595d79f2459048e59ff4784f6bfb600ba
Author: Dan Williams <djbw@kernel.org>

PCI/TSM: Add device interface security LOCKED support

This patch implements the LOCK/UNLOCK operations in pci_tsm_ops to allow a VM
to manage the security state of assigned devices. It provides the lock and
unlock sysfs attributes to transition the device between TDISP LOCKED and
UNLOCKED states.

> diff --git a/drivers/pci/tsm/core.c b/drivers/pci/tsm/core.c
> index 220842df42bc5..590e19f6c2b80 100644
> --- a/drivers/pci/tsm/core.c
> +++ b/drivers/pci/tsm/core.c

[ ... ]

> @@ -531,6 +552,125 @@ static ssize_t dsm_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(dsm);
>  
> +/**
> + * pci_tsm_unlock() - Transition TDI from LOCKED/RUN to UNLOCKED
> + * @pdev: TDI device to unlock
> + *
> + * Returns void, requires all callers to have satisfied dependencies like making
> + * sure the device is locked and detached from its driver.
> + */
> +static void pci_tsm_unlock(struct pci_dev *pdev)
> +{
> +	lockdep_assert_held_write(&pci_tsm_rwsem);
> +	device_lock_assert(&pdev->dev);
> +
> +	if (dev_WARN_ONCE(&pdev->dev, pdev->dev.driver,
> +			  "unlock attempted on driver attached device\n"))
> +		return;
> +

[Severity: High]
If pci_tsm_unlock() aborts early here because a driver is attached, pdev->tsm
is not cleared.

During subsystem teardown, __pci_tsm_destroy() calls pci_tsm_unlock(). If it
aborts here, does pdev->tsm remain pointing to the context while the TSM
provider unregisters and frees its memory, leaving a dangling pointer?

> +	device_cc_reject(&pdev->dev);
> +	to_pci_tsm_ops(pdev->tsm)->unlock(pdev->tsm);
> +	pdev->tsm = NULL;
> +}
> +
> +static int pci_tsm_lock(struct pci_dev *pdev, struct tsm_dev *tsm_dev)

[ ... ]

> +static ssize_t lock_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t len)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int rc, id;
> +
> +	rc = sscanf(buf, "tsm%d\n", &id);
> +	if (rc != 1)
> +		return -EINVAL;
> +
> +	ACQUIRE(rwsem_write_kill, lock)(&pci_tsm_rwsem);
> +	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &lock)))
> +		return rc;
> +
> +	if (pdev->tsm)
> +		return -EBUSY;
> +
> +	struct tsm_dev *tsm_dev __free(put_tsm_dev) = find_tsm_dev(id);
> +	if (!is_devsec_tsm(tsm_dev))
> +		return -ENXIO;
> +
> +	rc = pci_tsm_lock(pdev, tsm_dev);

[Severity: High]
This is a pre-existing issue, but can lock_store() attach a PCI device to a
TSM that is actively being destroyed?

In tsm_unregister(), pci_tsm_unregister() sweeps the devices and drops
pci_tsm_rwsem. Before the device is actually unregistered and freed by
device_unregister(), could lock_store() acquire the rwsem, look up the dying
tsm_dev via find_tsm_dev(), and attach it to a PCI device?

This seems like it could lead to a use-after-free when the TSM memory is freed
shortly after.

> +	if (rc)
> +		return rc;
> +	return len;
> +}
> +

[ ... ]

> +static ssize_t unlock_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t len)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct tsm_dev *tsm_dev;
> +	int rc;
> +
> +	ACQUIRE(rwsem_write_kill, lock)(&pci_tsm_rwsem);
> +	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &lock)))
> +		return rc;
> +
> +	if (!pdev->tsm)
> +		return -EINVAL;
> +

[Severity: High]
Does unlock_store() need to verify that pdev->tsm is a devsec context before
proceeding?

A PCI device can hold only one TSM context at a time, but if it has both
capabilities, both sets of sysfs attributes are visible. If a user writes to
unlock when the device has a link context, pci_tsm_unlock() executes and
dereferences ops->unlock(). Since link TSMs do not implement unlock, this
pointer is NULL, potentially causing a kernel panic.

Similarly, does the existing disconnect_store() verify if the context is a
link context? If disconnect is written to when the device has a devsec
context, __pci_tsm_disconnect() attempts to cast via to_pci_tsm_pf0(),
receiving NULL, and then dereferences it via guard(mutex)(&tsm_pf0->lock).

> +	tsm_dev = pdev->tsm->tsm_dev;
> +	if (!sysfs_streq(buf, dev_name(&tsm_dev->dev)))
> +		return -EINVAL;
> +
> +	ACQUIRE(device_intr, dev_lock)(&pdev->dev);
> +	if ((rc = ACQUIRE_ERR(device_intr, &dev_lock)))
> +		return rc;
> +
> +	if (pdev->dev.driver)
> +		return -EBUSY;
> +
> +	pci_tsm_unlock(pdev);
> +
> +	return len;
> +}
> +static DEVICE_ATTR_WO(unlock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705220819.2472765-1-djbw@kernel.org?part=10