[email protected] writes: > 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? >
None of these concerns are valid because cc_shared_granule_size() defaults to PAGE_SIZE on non-CC platforms. -aneesh
