Re: [PATCH 13/15] PCI, device core: Add private memory access for DEVICE_TRUST_TCB

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing pci_tsm_disable_dma() in pci_dma_configure() error path and pci_dma_cleanup() to revoke TCB DMA access.
- [Low] Invalid syntax in __must_hold attribute for pci_tsm_enable_dma and pci_tsm_disable_dma.
--

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

PCI, device core: Add private memory access for DEVICE_TRUST_TCB

This commit introduces the DEVICE_TRUST_TCB trust level to allow devices
within the Trusted Compute Boundary to access private memory. It updates
the PCI DMA configuration to validate DMA access with a TSM driver when a
device is TCB-trusted.

> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index f36778e62ac1a..e6a4bc3ca9b06 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c

[ ... ]

> @@ -1688,16 +1690,21 @@ static int pci_dma_configure(struct device *dev)
>  	 * the standard ACS capability but still support ACS via those
>  	 * quirks.
>  	 */
> -	pci_enable_acs(to_pci_dev(dev));
> -
> -	pci_put_host_bridge_device(bridge);
> +	pci_enable_acs(pdev);
>  
>  	/* @drv may not be valid when we're called from the IOMMU layer */
> -	if (!ret && drv && !to_pci_driver(drv)->driver_managed_dma) {
> +	if (ret || !drv)
> +		return ret;
> +
> +	if (device_tcb_trusted(dev))
> +		ret = pci_tsm_enable_dma(pdev);
> +
> +	if (!ret && !to_pci_driver(drv)->driver_managed_dma)
>  		ret = iommu_device_use_default_domain(dev);
> -		if (ret)
> -			arch_teardown_dma_ops(dev);
> -	}
> +
> +	/* undo {of,acpi}_dma_configure() */
> +	if (ret)
> +		arch_teardown_dma_ops(dev);

[Severity: High]
If iommu_device_use_default_domain() fails, does this error path need to
call pci_tsm_disable_dma() to revoke the TCB DMA access granted above?

Also, when the device driver unbinds, should pci_dma_cleanup() be updated
to call pci_tsm_disable_dma() as well? 

Without these cleanup calls, it appears the TSM could incorrectly maintain
DMA authorization for a device even after a probe failure or driver unbind.

[Severity: Low]
On a related note regarding the TSM core API modified in this series, are
the __must_hold() attributes in drivers/pci/tsm/core.c syntactically
correct?

drivers/pci/tsm/core.c:pci_tsm_enable_dma() {
    ...
    int pci_tsm_enable_dma(struct pci_dev *pdev)
        __must_hold(&pdev->dev->mutex)

Since dev is a nested struct within struct pci_dev, not a pointer, 
shouldn't this be &pdev->dev.mutex to avoid compiler errors? The same
appears to apply to pci_tsm_disable_dma().

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