Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] irqchip/gic-v3-its: buddy allocator corruption due to discarded shared_size --
commit d897529703a71c914b0334abcb563ffb37979469 Author: Aneesh Kumar K.V (Arm) <[email protected]> irqchip/gic-v3-its: Allocate shared tables using CoCo shared memory allocator This commit updates the ITS allocator to route page allocations through alloc_cc_shared_pages_node(). It reconstructs common allocation metadata from the original ITS order and the architecture granule size on release. > diff --git a/drivers/irqchip/irq-gic-v3-its.c > b/drivers/irqchip/irq-gic-v3-its.c > index ac5507acb1a0c..4814161df02ed 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c [ ... ] > @@ -214,25 +213,13 @@ static gfp_t gfp_flags_quirk; > static struct page *its_alloc_pages_node(int node, gfp_t gfp, > unsigned int order) > { > - struct page *page; > - int ret = 0; > - > - page = alloc_pages_node(node, gfp | gfp_flags_quirk, order); > - > - if (!page) > - return NULL; > + struct cc_shared_pages mem; > > - ret = set_memory_decrypted((unsigned long)page_address(page), > - 1 << order); > - /* > - * If set_memory_decrypted() fails then we don't know what state the > - * page is in, so we can't free it. Instead we leak it. > - * set_memory_decrypted() will already have WARNed. > - */ > - if (ret) > + if (alloc_cc_shared_pages_node(node, gfp | gfp_flags_quirk, > + PAGE_SIZE << order, &mem)) > return NULL; > > - return page; > + return mem.page; > } [Severity: High] Is it safe to discard mem.shared_size here? The API contract seems to require callers to retain the true allocated size so it can be passed back when freeing the pages. > static struct page *its_alloc_pages(gfp_t gfp, unsigned int order) > @@ -242,13 +229,15 @@ static struct page *its_alloc_pages(gfp_t gfp, unsigned > int order) > > static void its_free_pages(void *addr, unsigned int order) > { > - /* > - * If the memory cannot be encrypted again then we must leak the pages. > - * set_memory_encrypted() will already have WARNed. > - */ > - if (set_memory_encrypted((unsigned long)addr, 1 << order)) > + struct cc_shared_layout layout; > + struct cc_shared_pages mem; > + > + if (WARN_ON(cc_shared_calc_layout(PAGE_SIZE << order, &layout))) > return; > - free_pages((unsigned long)addr, order); > + > + mem.page = virt_to_page(addr); > + mem.shared_size = layout.shared_size; > + free_cc_shared_pages(&mem); > } [Severity: High] Does recalculating the size here cause a mismatch on non-CC platforms? When memory encryption is disabled, alloc_cc_shared_pages_node() does not round up the allocation to the granule size; it simply allocates the requested order. However, cc_shared_calc_layout() unconditionally rounds up to the architectural granule size. If the granule size exceeds the requested allocation size, free_cc_shared_pages() will call __free_pages() with a higher order than was actually allocated. Can this corrupt the buddy allocator by freeing unallocated pages? > static struct gen_pool *itt_pool; > @@ -273,7 +262,8 @@ static void *itt_alloc_pool(int node, int size) > if (!page) > break; > > - gen_pool_add(itt_pool, (unsigned long)page_address(page), > PAGE_SIZE, node); > + gen_pool_add(itt_pool, (unsigned long)page_address(page), > + cc_shared_granule_size(), node); > } while (!addr); > > return (void *)addr; [Severity: High] Will adding a hardcoded granule size to the generic pool advertise unallocated out-of-bounds memory to consumers? On non-CC platforms, the underlying allocation size is just a single page, but this adds the full cc_shared_granule_size() bytes to the pool. Could this lead to pool consumers accessing memory beyond what was actually allocated? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
