Re: [PATCH 12/15] PCI/TSM: Add device interface security DMA enable/disable

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing NULL pointer check for optional `ops->disable_dma` in `pci_tsm_disable_dma()`.
- [Medium] TSM flags are defined as bit masks instead of bit numbers, misusing bitops APIs and creating a latent out-of-bounds write hazard.
- [Low] Invalid member access syntax in `__must_hold()` lock annotations causes build failures for static analysis tools.

Pre-existing issues:
- [High] Missing NULL pointer checks for optional TSM operations (`run`, `unlock`) in the TSM core.
--

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

PCI/TSM: Add device interface security DMA enable/disable

This patch adds helper functions to enable and disable device access to
private memory for the PCI TSM interface. It also introduces a new flag
to track when DMA access is enabled for a given device.

> diff --git a/drivers/pci/tsm/core.c b/drivers/pci/tsm/core.c
> --- a/drivers/pci/tsm/core.c
> +++ b/drivers/pci/tsm/core.c
> @@ -754,6 +754,48 @@ static ssize_t unlock_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_WO(unlock);
>  
> +/* pci_dma_configure() helper to finalize access to private memory. */
> +int pci_tsm_enable_dma(struct pci_dev *pdev) __must_hold(&pdev->dev->mutex)

[Severity: Low]
Does this __must_hold() annotation cause build failures with static analysis
tools?

The dev field in struct pci_dev is an embedded struct device, not a
pointer, so it looks like this should use the dot operator instead of the
arrow operator for accessing the mutex.

> +{
> +	struct device *dev = &pdev->dev;
> +	const struct pci_tsm_ops *ops;
> +	int rc;

[ ... ]

> +	ops = to_pci_tsm_ops(pdev->tsm);
> +	if (!ops->enable_dma)
> +		return 0;
> +
> +	rc = ops->enable_dma(pdev);
> +	if (rc == 0)
> +		set_bit(PCI_TSM_F_DMA, &pdev->tsm->flags);

[Severity: Medium]
Can this corrupt memory?

The set_bit() function expects a bit number as its argument, but the
PCI_TSM_F_DMA flag is defined as a bit mask. Passing the evaluated mask
value instead of the bit index causes it to set the wrong bit in the flags
field.

If a future flag shifts by 64, wouldn't passing 64 to set_bit() write past
the end of the unsigned long flags field and silently corrupt adjacent
memory?

> +	return rc;
> +}
> +
> +/*
> + * pci_dma_cleanup() helper to block private memory access as device is
> + * going idle
> + */
> +void pci_tsm_disable_dma(struct pci_dev *pdev) __must_hold(&pdev->dev->mutex)

[Severity: Low]
Is this __must_hold() annotation affected by the same embedded struct access
issue mentioned in pci_tsm_enable_dma()?

> +{
> +	const struct pci_tsm_ops *ops;
> +
> +	if (!pdev->tsm)
> +		return;
> +
> +	if (!test_and_clear_bit(PCI_TSM_F_DMA, &pdev->tsm->flags))
> +		return;
> +
> +	ops = to_pci_tsm_ops(pdev->tsm);
> +	ops->disable_dma(pdev);

[Severity: High]
Will this crash if a backend driver implements enable_dma but leaves
disable_dma as NULL?

The disable_dma pointer is unconditionally dereferenced here, whereas
pci_tsm_enable_dma() checks for the presence of the enable_dma callback
before calling it.

> +}

[ ... ]

> diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
> --- a/include/linux/pci-tsm.h
> +++ b/include/linux/pci-tsm.h

[ ... ]

> @@ -102,6 +104,8 @@ struct pci_tdi {
>  
>  /* Private operation acknowledged, future ioremap will use private alias */
>  #define PCI_TSM_F_ACCEPT (1UL << 0)
> +/* DMA access to private memory enabled */
> +#define PCI_TSM_F_DMA (1UL << 1)

[Severity: Medium]
Is this flag intended to be used with atomic bitops?

Because it is defined as a bit mask instead of a bit number, using it with
functions like set_bit() or test_and_clear_bit() will operate on bit 2
instead of bit 1.

[ ... ]

While reviewing this patch, I also noticed a potential issue in the
existing TSM core code:

drivers/pci/tsm/core.c:pci_tsm_accept() {
    ...
    rc = to_pci_tsm_ops(pdev->tsm)->run(pdev);
    if (rc)
        return rc;
    ...
}

[Severity: High]
This is a pre-existing issue, but could this cause a kernel panic if a
backend driver omits the optional run or unlock callbacks?

The code in pci_tsm_accept() and pci_tsm_unlock() seems to unconditionally
dereference these pointers without checking if they are NULL, similar to
the disable_dma issue mentioned above.

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