Re: [PATCH 15/15] PCI/TSM: Add relative MMIO offset support?

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The BAR mask calculation in `pci_tsm_mmio_alloc()` uses a 32-bit `unsigned long` for `mask` on 32-bit architectures, corrupting 64-bit `tsm_offset` calculations.
- [High] A 32-bit integer overflow when calculating `size` in `pci_tsm_mmio_alloc()` silently truncates MMIO ranges of 4GB or larger.
--

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

PCI/TSM: Add relative MMIO offset support?

The RMM specification documents the expectation that the MMIO_REPORTING_OFFSET
chosen for TDISP Interface Reports is always BAR aligned. This patch adds
support to ask the TSM drivers for the offset calculation scheme if it
deviates from this assumption.

Cc: Alexey Kardashevskiy <aik@amd.com>
Cc: Xu Yilun <yilun.xu@linux.intel.com>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
Signed-off-by: Dan Williams <djbw@kernel.org>

> diff --git a/drivers/pci/tsm/core.c b/drivers/pci/tsm/core.c
> index 9ac216ad896d9..19ad35f2da4a9 100644
> --- a/drivers/pci/tsm/core.c
> +++ b/drivers/pci/tsm/core.c
> @@ -643,13 +643,15 @@ struct pci_tsm_devif_report {
>  /**
>   * pci_tsm_mmio_alloc() - allocate encrypted MMIO range descriptor
>   * @pdev: device owner of MMIO ranges
> + * @scheme: allow the low level TSM driver to hint the offset calc scheme
>   *
>   * Return: the encrypted MMIO range descriptor on success, NULL on failure

[Severity: High]
This is a pre-existing issue, but I noticed a potential 32-bit
multiplication overflow in this function.

When calculating the size in pci_tsm_mmio_alloc():
    u64 size = __le32_to_cpu(mmio_data->nr_pfns) * SZ_4K;

Since both operands are 32-bit, will this overflow if nr_pfns is 1,048,576
(representing 4GB) or greater? The overflow would occur before being
promoted and assigned to the 64-bit size variable, causing MMIO ranges of
4GB or larger to be silently truncated.

>   *
>   * Assumes that this is called within the live lifetime of a PCI device's
>   * association with a low level TSM.
>   */
> -struct pci_tsm_mmio *pci_tsm_mmio_alloc(struct pci_dev *pdev)
> +struct pci_tsm_mmio *pci_tsm_mmio_alloc(struct pci_dev *pdev,
> +					enum tdisp_offset_scheme scheme)
>  {

[ ... ]

> @@ -712,10 +714,14 @@ struct pci_tsm_mmio *pci_tsm_mmio_alloc(struct pci_dev *pdev)
>  			last_bar = bar;
>  
>  			/*
> -			 * Determine the obfuscated base of the BAR. BAR
> -			 * offsets are never obfuscated.
> +			 * Either the first range per bar always maps
> +			 * the start of the BAR, or the reporting_offset
> +			 * is BAR size aligned.
>  			 */
> -			reporting_bar_base = tsm_offset & ~mask;
> +			if (scheme == TDISP_OFFSET_RELATIVE)
> +				reporting_bar_base = tsm_offset;
> +			else
> +				reporting_bar_base = tsm_offset & ~mask;

[Severity: High]
This is a pre-existing issue, but could this bitwise AND corrupt 64-bit
calculations on 32-bit architectures?

Looking at the definition of mask earlier in pci_tsm_mmio_alloc():
    unsigned long mask = pci_resource_len(pdev, bar) - 1;

Since pci_resource_len() returns a 64-bit resource_size_t on PAE systems,
it gets truncated to a 32-bit unsigned long mask on 32-bit architectures.
When ~mask is evaluated, it results in a 32-bit value.

Would applying bitwise AND with the 64-bit u64 tsm_offset here cause
zero-extension of ~mask, explicitly clearing all bits above 4GB in
tsm_offset?

>  		} else if (tsm_offset < last_reporting_end) {

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