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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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),
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/mm/cma.c b/mm/cma.c
index a13ce4999b39f..4bc6eef8b6060 100644
--- a/mm/cma.c
+++ b/mm/cma.c
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ 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);
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+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)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+{
+ 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