Re: [PATCH v5 07/19] mm: x86: Untag addresses in EXECMEM_ROX related pointer arithmetic

[Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>]

On 2025-08-28 at 12:50:19 +0300, Mike Rapoport wrote:
>On Mon, Aug 25, 2025 at 10:24:32PM +0200, Maciej Wieczor-Retman wrote:
>> ARCH_HAS_EXECMEM_ROX was re-enabled in x86 at Linux 6.14 release.
>> Related code has multiple spots where page virtual addresses end up used
>> as arguments in arithmetic operations. Combined with enabled tag-based
>> KASAN it can result in pointers that don't point where they should or
>> logical operations not giving expected results.
>> 
>> vm_reset_perms() calculates range's start and end addresses using min()
>> and max() functions. To do that it compares pointers but some are not
>> tagged - addr variable is, start and end variables aren't.
>> 
>> within() and within_range() can receive tagged addresses which get
>> compared to untagged start and end variables.
>> 
>> Reset tags in addresses used as function arguments in min(), max(),
>> within().
>> 
>> execmem_cache_add() adds tagged pointers to a maple tree structure,
>> which then are incorrectly compared when walking the tree. That results
>> in different pointers being returned later and page permission violation
>> errors panicking the kernel.
>> 
>> Reset tag of the address range inserted into the maple tree inside
>> execmem_cache_add().
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v5:
>> - Remove the within_range() change.
>> - arch_kasan_reset_tag -> kasan_reset_tag.
>> 
>> Changelog v4:
>> - Add patch to the series.
>> 
>>  mm/execmem.c | 2 +-
>>  mm/vmalloc.c | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/mm/execmem.c b/mm/execmem.c
>> index 0822305413ec..f7b7bdacaec5 100644
>> --- a/mm/execmem.c
>> +++ b/mm/execmem.c
>> @@ -186,7 +186,7 @@ static DECLARE_WORK(execmem_cache_clean_work, execmem_cache_clean);
>>  static int execmem_cache_add_locked(void *ptr, size_t size, gfp_t gfp_mask)
>>  {
>>  	struct maple_tree *free_areas = &execmem_cache.free_areas;
>> -	unsigned long addr = (unsigned long)ptr;
>> +	unsigned long addr = (unsigned long)kasan_reset_tag(ptr);
>
>Thinking more about it, we anyway reset tag in execmem_alloc() and return
>untagged pointer to the caller. Let's just move kasan_reset_tag() to
>execmem_vmalloc() so that we always use untagged pointers. Seems more
>robust to me.

Sure, I'll test if it works and change it :)

>
>>  	MA_STATE(mas, free_areas, addr - 1, addr + 1);
>>  	unsigned long lower, upper;
>>  	void *area = NULL;
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index 6dbcdceecae1..c93893fb8dd4 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -3322,7 +3322,7 @@ static void vm_reset_perms(struct vm_struct *area)
>>  	 * the vm_unmap_aliases() flush includes the direct map.
>>  	 */
>>  	for (i = 0; i < area->nr_pages; i += 1U << page_order) {
>> -		unsigned long addr = (unsigned long)page_address(area->pages[i]);
>> +		unsigned long addr = (unsigned long)kasan_reset_tag(page_address(area->pages[i]));
>
>This is not strictly related to execemem, there may other users of
>VM_FLUSH_RESET_PERMS.
>
>Regardless, I wonder how this works on arm64 with tags enabled?

Hmm, good point, I'll check it out in qemu if this function is called on arm64.

However this issue didn't pop up for me before 6.14 when EXECMEM_ROX was
enabled, so maybe it just didn't hit tagged pages before? I'll try to recheck
that on x86 too.

>Also, it's not the only place in the kernel that does (unsigned
>long)page_address(page). Do other sites need to reset the tag as well?

This place is special in the sense that it does "start = min(addr, start)" and
"end = max(addr, end)" just a few lines later. And start and end seem to always be
untagged, while addr sometimes gets tagged. So with software KASAN and vmalloc
support enabled it would get the final start and end values wrong and then a
page permission error would happen someplace else.

I don't think all other page_address(page) sites need resetting, but I'll double
check if there is any pointer arithmetic there.

>
>>  
>>  		if (addr) {
>>  			unsigned long page_size;
>> -- 
>> 2.50.1
>> 
>
>-- 
>Sincerely yours,
>Mike.

-- 
Kind regards
Maciej Wieczór-Retman