Re: [PATCH v5 05/10] mm/cma: Introduce cma_alloc_at() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Missing bounds check on `offset` and `count` in `cma_range_alloc_at()` allows out-of-bounds heap read/write.
- [High] Failure to scale offsets and counts by `cma->order_per_bit` causes state corruption and memory leaks.
- [High] `__cma_alloc_at_frozen()` illogically applies the exact same relative `offset` to every disjoint range.
- [Medium] Misleading parameter naming (`pfn` vs `offset`) in `cma_alloc_at()` leads to dangerous misinterpretations.
--

--- Patch [5]: [PATCH v5 05/10] mm/cma: Introduce cma_alloc_at() API ---
commit 86953b3fddeca8ceb93fb736c755a0bfda094530
Author: Thierry Reding <treding@nvidia.com>

mm/cma: Introduce cma_alloc_at() API
    
This API can be used to allocate a number of CMA pages starting at a
fixed offset. This is useful, for example, if the CMA area is used as
backing storage for a nested allocator that has stricter requirements
than CMA itself.

> diff --git a/include/trace/events/cma.h b/include/trace/events/cma.h
> index 37195edf24986..00b622a9da97b 100644
> --- a/include/trace/events/cma.h
> +++ b/include/trace/events/cma.h
[ ... ]
> @@ -132,6 +132,69 @@ TRACE_EVENT(cma_alloc_busy_retry,
>  		  __entry->align)
>  );
>  
> +TRACE_EVENT(cma_alloc_at_start,
> +
> +	TP_PROTO(const char *name, unsigned long pfn,

[Severity: Medium]
Could this parameter name cause confusion? The function signature in cma.h uses
offset, but this tracepoint logs it as pfn. If a caller mistakenly passes an
absolute physical frame number instead of a relative offset, could this lead
to an incorrect allocation address computation later in cma_range_alloc_at()?

> +		 unsigned long request_count, unsigned long available_count,
> +		 unsigned long total_count),
[ ... ]
> diff --git a/mm/cma.c b/mm/cma.c
> index a13ce4999b39f..4bc6eef8b6060 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
[ ... ]
> +static int cma_range_alloc_at(struct cma *cma, struct cma_memrange *cmr,
> +			      unsigned long offset, unsigned long count,
> +			      struct page **pagep, gfp_t gfp)
> +{
> +	struct page *page = NULL;
> +	unsigned long pfn;
> +	int ret = -EBUSY;
> +
> +	spin_lock_irq(&cma->lock);
> +
> +	/*
> +	 * If the request is larger than the available number of pages, stop
> +	 * right away.
> +	 */
> +	if (count > cma->available_count)
> +		goto unlock;
> +
> +	ret = bitmap_allocate(cmr->bitmap, offset, count);

[Severity: Critical]
Does this code allow an out-of-bounds read or write on the bitmap? It does not
appear to check if offset + count is within cma_bitmap_maxno(cma, cmr).
Because bitmap_allocate() scans and sets bits without knowing the allocation
bounds, a large offset + count could overrun the bitmap.

[Severity: High]
Does this code treat page counts as raw bit indices? The offset and count
parameters represent pages, but are passed directly to bitmap_allocate()
without scaling by cma->order_per_bit.

> +	if (ret < 0)
> +		goto unlock;
> +
> +	pfn = cmr->base_pfn + offset;

[Severity: High]
If offset represents raw pages, shouldn't it be consistent with how the
bitmap was updated above? If cma->order_per_bit is non-zero, this treats
offset as pages while the bitmap logic treated it as bits.

> +	page = pfn_to_page(pfn);
[ ... ]
> +free:
> +	/* we need to reacquire the lock to clean up the internal state */
> +	spin_lock_irq(&cma->lock);
> +	cma->available_count += count;
> +clear:
> +	bitmap_clear(cmr->bitmap, offset, count);

[Severity: High]
Does this similarly fail to convert the page offset and count to bit indices?
If cma_release() later frees these pages, it will convert the PFNs to bits,
potentially clearing different bits than were set and permanently leaking the
original allocation.

> +unlock:
> +	spin_unlock_irq(&cma->lock);
> +	return ret;
> +}
> +
> +static struct page *__cma_alloc_at_frozen(struct cma *cma, unsigned long offset,
> +					  unsigned long count, gfp_t gfp)
> +{
[ ... ]
> +	for (r = 0; r < cma->nranges; r++) {
> +		page = NULL;
> +
> +		ret = cma_range_alloc_at(cma, &cma->ranges[r], offset, count,
> +					 &page, gfp);

[Severity: High]
Does this apply the same unadjusted offset to every physical memrange? If the
allocation falls back to a subsequent range, it will attempt the allocation at
ranges[r].base_pfn + offset. If offset is meant to be a logical index across
the contiguous CMA area, it seems it should be decremented by the size of
preceding ranges as the loop iterates.

> +		if (ret != -EBUSY || page)
> +			break;
> +	}
[ ... ]
> +struct page *cma_alloc_at(struct cma *cma, unsigned long pfn,
> +			  unsigned long count, bool no_warn)

[Severity: Medium]
Is pfn the correct name for this parameter? It appears to be used as a
relative offset in cma_range_alloc_at(). Passing an absolute physical frame
number could result in computing cmr->base_pfn + pfn, producing an incorrect
physical address.

> +{
> +	struct page *page;
> +
> +	page = cma_alloc_at_frozen(cma, pfn, count, no_warn);
> +	if (page)
> +		set_pages_refcounted(page, count);
> +
> +	return page;
> +}
> +EXPORT_SYMBOL_GPL(cma_alloc_at);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814-tegra-vpr-v5-0-71832b5d0246@nvidia.com?part=5