Re: [RFC PATCH v8 04/14] irqchip/gic-v3-its: Allocate shared tables using CoCo shared memory allocator

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

sashiko-bot@kernel.org 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) <aneesh.kumar@kernel.org>
>
> 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