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 <[email protected]> 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/[email protected]?part=5
