Re: [PATCH v2 02/30] iommu/io-pgtable-arm: introduce iova_to_phys_length in io_pgtable_ops

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `PHYS_ADDR_MAX` error sentinel collides with a legitimate 32-bit physical address, breaking translations for PA 0xFFFFFFFF.
- [Medium] The documentation for `iova_to_phys_length` contradicts its implementation and callers' expectations, creating a dangerous API contract mismatch.

Pre-existing issues:
- [High] If `iova` is `ULONG_MAX`, the page table walk loop silently skips execution and returns success, leading to undefined behavior and uninitialized stack variable usage.
- [High] Time-of-Check to Time-of-Use (TOCTOU) race and potential torn read in lockless page table walk.
--

commit 8479ee64fd320d9e1e651dd5c19fe22d7dbeb287
Author: Guanghui Feng <guanghuifeng@linux.alibaba.com>

iommu/io-pgtable-arm: introduce iova_to_phys_length in io_pgtable_ops

This commit adds iova_to_phys_length to struct io_pgtable_ops and implements
it for the ARM LPAE backend. The new function returns the physical address and
the mapped length at the resolved level.

> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 476c0e25631af..2b94e0563e214 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -765,8 +765,21 @@ static int visit_iova_to_phys(struct io_pgtable_walk_data *walk_data, int lvl,
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but is there a risk of a torn read or
time-of-check to time-of-use (TOCTOU) race in the visitor callback?

While io_pgtable_visit() safely reads the entry via READ_ONCE(*ptep) to
protect against concurrent modification, it passes the raw ptep pointer to
visit_iova_to_phys(). The callback then performs a second unprotected read
via data->pte = *ptep.

If a concurrent page table modification occurs, the callback could capture a
new value while io_pgtable_visit() validated the old one. Additionally, since
arm_lpae_iopte is 64-bit, reading it without READ_ONCE() on 32-bit ARM might
allow the compiler to emit multiple 32-bit loads, potentially resulting in a
torn read.

>  
> +static phys_addr_t arm_lpae_iova_to_phys_length(struct io_pgtable_ops *ops,
> +						 unsigned long iova,
> +						 size_t *mapped_length);
> +
> +static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
> +					 unsigned long iova)
> +{
> +	phys_addr_t phys = arm_lpae_iova_to_phys_length(ops, iova, NULL);
> +
> +	return (phys == PHYS_ADDR_MAX) ? 0 : phys;
> +}

[Severity: High]
Could this return logic incorrectly mask a valid physical address?

On 32-bit platforms where phys_addr_t is 32 bits, PHYS_ADDR_MAX evaluates to
0xFFFFFFFF. If a valid MMIO or RAM region is mapped exactly at the top of the
physical address space, returning 0xFFFFFFFF from
arm_lpae_iova_to_phys_length() would be treated as an error here, and
incorrectly return 0 instead.

> +
> +static phys_addr_t arm_lpae_iova_to_phys_length(struct io_pgtable_ops *ops,
> +						 unsigned long iova,
> +						 size_t *mapped_length)
>  {
>  	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
>  	struct iova_to_phys_data d;
> @@ -776,13 +789,18 @@ static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
>  		.addr = iova,
>  		.end = iova + 1,
>  	};

[Severity: High]
This is a pre-existing issue, but what happens to the walk_data.end
calculation if iova is ULONG_MAX?

If iova is exactly ULONG_MAX, walk_data.end = iova + 1 will overflow to 0.
During the __arm_lpae_iopte_walk() loop, the condition
(walk_data->addr < walk_data->end) will evaluate to (ULONG_MAX < 0), which
is immediately false.

Because the loop exits without calling the visit_iova_to_phys() callback, the
function would assume success and proceed to calculate the block_size using an
uninitialized d.lvl variable from the stack.

[ ... ]

> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index e19872e37e067..42bcdd309b889 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -203,6 +203,10 @@ struct arm_lpae_io_pgtable_walk_data {
>   * @map_pages:    Map a physically contiguous range of pages of the same size.
>   * @unmap_pages:  Unmap a range of virtually contiguous pages of the same size.
>   * @iova_to_phys: Translate iova to physical address.
> + * @iova_to_phys_length: Translate iova to physical address and return the
> + *			  remaining mapped length from iova to the end of the
> + *			  mapping entry via @mapped_length. If @mapped_length is
> + *			  NULL, only the physical address is returned.

[Severity: Medium]
Does this documentation accurately describe the implementation?

The comment specifies that mapped_length outputs the remaining mapped length
from the IOVA to the end of the entry. However, the
arm_lpae_iova_to_phys_length() implementation returns the full block size of
the PTE mapping without subtracting the IOVA offset.

Could this mismatch mislead developers implementing future backends?

>   * @pgtable_walk: (optional) Perform a page table walk for a given iova.
>   * @read_and_clear_dirty: Record dirty info per IOVA. If an IOVA is dirty,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602104637.1219810-1-guanghuifeng@linux.alibaba.com?part=2