Re: [PATCH v9 09/10] firmware: arm_ffa: Introduce ffa-lend-pool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing TLB flush after invalidating direct map PTEs allows speculative reads.
- [High] Lockless TOCTOU data race on `dev->cma_area` leading to potential NULL pointer dereference.
- [High] Unsafe struct page pointer arithmetic across sparse memory section boundaries.
--

commit 8e8290aac241785be21b825c484422d3877b1940
Author: Vincent Donnefort <vdonnefort@google.com>

firmware: arm_ffa: Introduce ffa-lend-pool

This commit introduces the arm,ffa-lend-pool reserved-memory CMA driver
to prevent fatal CPU speculative reads from Non-Secure memory by unmapping
pages before lending and restoring them upon reclaim.

> diff --git a/drivers/firmware/arm_ffa/lend_pool.c b/drivers/firmware/arm_ffa/lend_pool.c
> new file mode 100644
> index 0000000000000..67a36f5d5aa68
> --- /dev/null
> +++ b/drivers/firmware/arm_ffa/lend_pool.c
[ ... ]
> +static bool ffa_lend_pool_contains(struct device *dev, phys_addr_t addr, size_t size)
> +{
> +	phys_addr_t base, end;
> +
> +	if (!dev)
> +		return false;
> +
> +	guard(rcu)();
> +
> +	if (xa_load(&ffa_lend_devices, (unsigned long)dev) != dev)
> +		return false;
> +
> +	if (WARN_ON_ONCE(!dev->cma_area))
> +		return false;
> +
> +	base = cma_get_base(dev->cma_area);

[Severity: High]
Could there be a time-of-check to time-of-use (TOCTOU) race here?

If ffa_lend_pool_device_release() is called concurrently (e.g., during
device unbind) and clears the pointer without waiting for an RCU grace
period:

ffa_lend_pool_device_release() {
    xa_erase(&ffa_lend_devices, (unsigned long)dev);
    dev->cma_area = NULL;
}

The compiler can emit two separate loads for dev->cma_area in
ffa_lend_pool_contains(). If the pointer is cleared between the
WARN_ON_ONCE() check and the cma_get_base() call, cma_get_base() might
dereference a NULL pointer.

> +	end = base + cma_get_size(dev->cma_area);
> +
> +	return addr >= base && (addr + size) <= end;
> +}
[ ... ]
> +int ffa_prepare_lend(struct device *dev, phys_addr_t addr, size_t size)
> +{
> +	unsigned long nr_pages = size >> PAGE_SHIFT;
> +	struct page *page;
> +	unsigned long i;
> +	int ret;
> +
> +	if (!ffa_lend_pool_contains(dev, addr, size))
> +		return -ENODEV;
> +
> +	page = pfn_to_page(PHYS_PFN(addr));
> +	for (i = 0; i < nr_pages; i++) {
> +		ret = __set_direct_map_invalid_noflush(page + i);

[Severity: High]
Is it safe to use contiguous pointer arithmetic (page + i) for struct page
iteration here?

On architectures where CONFIG_SPARSEMEM is enabled but
CONFIG_SPARSEMEM_VMEMMAP is not (such as 32-bit ARM, which the FF-A
transport supports), the struct page array is broken up into sections and
is not virtually contiguous.

If the CMA region crosses a memory section boundary (e.g., a 64MB region
crossing a 16MB or 32MB section boundary), would this arithmetic point to
unmapped memory or invalid section metadata?

> +
> +		if (ret) {
> +			while (i--)
> +				__set_direct_map_default_noflush(page + i);
> +
> +			return ret;
> +		}
> +	}
> +
> +	return 0;

[Severity: High]
Does this sequence leave stale unmapped entries in the TLB?

By returning success after __set_direct_map_invalid_noflush() without ever
calling flush_tlb_kernel_range() (or an equivalent TLB flush), can
speculative execution still use the cached TLB entries to read the memory?

If TrustZone transitions the memory to the Secure state while valid
translations remain cached, could these speculative accesses still cause
the fatal exceptions this patch intends to prevent?

> +}
> +EXPORT_SYMBOL_GPL(ffa_prepare_lend);
> +
> +/**
> + * ffa_lend_reclaimed() - Restore a reclaimed FF-A memory region
> + * @dev:	Device attached to the lend pool
> + * @addr:	Physical start address of the memory region
> + * @size:	Size in bytes
> + *
> + * Restores a memory range into the kernel's direct mapping. It must be called
> + * after a successful FF-A memory reclaim invocation.
> + */
> +void ffa_lend_reclaimed(struct device *dev, phys_addr_t addr, size_t size)
> +{
> +	unsigned long nr_pages = size >> PAGE_SHIFT;
> +	struct page *page;
> +	unsigned long i;
> +
> +	if (!ffa_lend_pool_contains(dev, addr, size))
> +		return;
> +
> +	page = pfn_to_page(PHYS_PFN(addr));
> +	for (i = 0; i < nr_pages; i++)
> +		__set_direct_map_default_noflush(page + i);

[Severity: High]
Similar to ffa_prepare_lend() above, does this struct page arithmetic
(page + i) risk crashing across sparse memory section boundaries?

> +}
> +EXPORT_SYMBOL_GPL(ffa_lend_reclaimed);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902104712.2399797-1-vdonnefort@google.com?part=9