Re: [PATCH 11/15] PCI/TSM: Add device interface security RUN support

[sashiko-bot@kernel.org]

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

New issues:
- [High] `pci_tsm_accept()` unconditionally invokes the `run` callback without verifying the TSM type or callback presence, leading to a NULL pointer dereference.
- [High] The `PCI_TSM_F_ACCEPT` flag is not explicitly cleared, allowing a reused context to bypass the TDISP RUN state transition.

Pre-existing issues:
- [High] Missing validation of TSM type in `unlock_store` and `disconnect_store` can cause NULL pointer dereferences.
- [High] Lock order inversion between `pci_tsm_rwsem` and `device_lock(&pdev->dev)` causes an ABBA deadlock.
--

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

PCI/TSM: Add device interface security RUN support

This commit introduces a sysfs trigger to transition a device from the TDISP
LOCKED state to the RUN state. It connects the accept trigger to a new run
operation in the pci_tsm_ops structure. The RUN state prepares the device for
private operation by enabling private MMIO access.

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

[ ... ]

> @@ -552,6 +552,89 @@ static ssize_t dsm_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(dsm);
>  
> +/**
> + * pci_tsm_accept() - accept a device for private MMIO operation
> + * @pdev: PCI device to accept
> + *
> + * "Accept" transitions the device interface to the RUN state, it is
> + * only suitable to make that transition from a known idle (no active
> + * DMA / MMIO mappings) state. The "driver detached" state is a coarse
> + * way to assert that requirement.
> + */
> +static int pci_tsm_accept(struct pci_dev *pdev)
> +{
> +	int rc;
> +
> +	ACQUIRE(rwsem_read_intr, lock)(&pci_tsm_rwsem);
> +	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &lock)))
> +		return rc;
> +
> +	if (!pdev->tsm)
> +		return -EINVAL;
> +
> +	ACQUIRE(device_intr, dev_lock)(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but does acquiring pci_tsm_rwsem and then
device_lock(&pdev->dev) cause an ABBA deadlock?

If a device driver's remove() callback is invoked holding device_lock, and it
calls pci_tsm_unbind(), which then attempts to acquire pci_tsm_rwsem, this
could invert the lock ordering. 

Concurrently, sysfs operations like unlock_store or lock_store acquire
pci_tsm_rwsem before device_lock, which could trigger a deadlock.

> +	if ((rc = ACQUIRE_ERR(device_intr, &dev_lock)))
> +		return rc;
> +
> +	if (pdev->dev.driver)
> +		return -EBUSY;
> +
> +	if (test_bit(PCI_TSM_F_ACCEPT, &pdev->tsm->flags))

[Severity: High]
Can a reused context bypass the TDISP RUN state transition because
PCI_TSM_F_ACCEPT is never explicitly cleared?

When a device is unlocked, the provider's ops->unlock() retains the
struct pci_tsm object in memory. When the device is locked again, the
core constructor pci_tsm_devsec_constructor() initializes the base fields
but does not clear flags. 

Does this allow a subsequent call to pci_tsm_accept() to observe the leftover
PCI_TSM_F_ACCEPT bit and return success without calling the hardware run
operation?

> +		return 0;
> +
> +	rc = to_pci_tsm_ops(pdev->tsm)->run(pdev);

[Severity: High]
Does this code unconditionally invoke the run callback without verifying the
TSM type or callback presence, leading to a NULL pointer dereference?

If a user space process writes to the accept sysfs attribute of a device that
is bound to a link TSM (which does not populate devsec_ops) or a devsec TSM
that omitted the run callback, it seems this would result in a kernel crash.

> +	if (rc)
> +		return rc;
> +
> +	set_bit(PCI_TSM_F_ACCEPT, &pdev->tsm->flags);
> +
> +	return 0;
> +}

[ ... ]

> @@ -568,9 +651,9 @@ static void pci_tsm_unlock(struct pci_dev *pdev)
>  			  "unlock attempted on driver attached device\n"))
>  		return;
>  
> -	device_cc_reject(&pdev->dev);
>  	to_pci_tsm_ops(pdev->tsm)->unlock(pdev->tsm);

[Severity: High]
This isn't a bug introduced by this patch, but could the lack of TSM type
validation in unlock_store and disconnect_store cause NULL pointer
dereferences?

If a user writes the name of a link TSM to the unlock attribute, or a devsec
TSM to the disconnect attribute, the sysfs handler matches the name. 

Since it fails to verify the specific type of operations supported by the TSM
before calling ops->unlock or ops->disconnect, could this lead to a crash?

>  	pdev->tsm = NULL;
> +	/* implicit clear of PCI_TSM_F_ACCEPT */

[Severity: High]
Is this assumption correct that un-assigning the pointer clears the memory
of the provider-managed flags field?

As asked above, if the struct pci_tsm object remains in memory and flags are
not cleared during initialization, it looks like the PCI_TSM_F_ACCEPT flag
could persist across locking cycles.

>  }

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