linux-kernel - Re: [PATCH v4 06/18] x86: Reset tag for virtual to physical address conversions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <tnswzssq2kyt3sla5enhzjmqh7m2xum6y7lprrimvrcajbqe7j@3cdk6tcdyjjy>
Date: Mon, 18 Aug 2025 07:29:29 +0200
From: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
To: Mike Rapoport <rppt@...nel.org>
CC: <nathan@...nel.org>, <arnd@...db.de>, <broonie@...nel.org>,
	<Liam.Howlett@...cle.com>, <urezki@...il.com>, <will@...nel.org>,
	<kaleshsingh@...gle.com>, <leitao@...ian.org>, <coxu@...hat.com>,
	<surenb@...gle.com>, <akpm@...ux-foundation.org>, <luto@...nel.org>,
	<jpoimboe@...nel.org>, <changyuanl@...gle.com>, <hpa@...or.com>,
	<dvyukov@...gle.com>, <kas@...nel.org>, <corbet@....net>,
	<vincenzo.frascino@....com>, <smostafa@...gle.com>,
	<nick.desaulniers+lkml@...il.com>, <morbo@...gle.com>,
	<andreyknvl@...il.com>, <alexander.shishkin@...ux.intel.com>,
	<thiago.bauermann@...aro.org>, <catalin.marinas@....com>,
	<ryabinin.a.a@...il.com>, <jan.kiszka@...mens.com>, <jbohac@...e.cz>,
	<dan.j.williams@...el.com>, <joel.granados@...nel.org>, <baohua@...nel.org>,
	<kevin.brodsky@....com>, <nicolas.schier@...ux.dev>, <pcc@...gle.com>,
	<andriy.shevchenko@...ux.intel.com>, <wei.liu@...nel.org>, <bp@...en8.de>,
	<ada.coupriediaz@....com>, <xin@...or.com>, <pankaj.gupta@....com>,
	<vbabka@...e.cz>, <glider@...gle.com>, <jgross@...e.com>, <kees@...nel.org>,
	<jhubbard@...dia.com>, <joey.gouly@....com>, <ardb@...nel.org>,
	<thuth@...hat.com>, <pasha.tatashin@...een.com>,
	<kristina.martsenko@....com>, <bigeasy@...utronix.de>,
	<lorenzo.stoakes@...cle.com>, <jason.andryuk@....com>, <david@...hat.com>,
	<graf@...zon.com>, <wangkefeng.wang@...wei.com>, <ziy@...dia.com>,
	<mark.rutland@....com>, <dave.hansen@...ux.intel.com>,
	<samuel.holland@...ive.com>, <kbingham@...nel.org>,
	<trintaeoitogc@...il.com>, <scott@...amperecomputing.com>,
	<justinstitt@...gle.com>, <kuan-ying.lee@...onical.com>, <maz@...nel.org>,
	<tglx@...utronix.de>, <samitolvanen@...gle.com>, <mhocko@...e.com>,
	<nunodasneves@...ux.microsoft.com>, <brgerst@...il.com>,
	<willy@...radead.org>, <ubizjak@...il.com>, <peterz@...radead.org>,
	<mingo@...hat.com>, <sohil.mehta@...el.com>, <linux-mm@...ck.org>,
	<linux-kbuild@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
	<x86@...nel.org>, <llvm@...ts.linux.dev>, <kasan-dev@...glegroups.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 06/18] x86: Reset tag for virtual to physical address
 conversions

Hi and thanks for looking at the patches :)

On 2025-08-14 at 10:15:09 +0300, Mike Rapoport wrote:
>On Tue, Aug 12, 2025 at 03:23:42PM +0200, Maciej Wieczor-Retman wrote:
>> Any place where pointer arithmetic is used to convert a virtual address
>> into a physical one can raise errors if the virtual address is tagged.
>> 
>> Reset the pointer's tag by sign extending the tag bits in macros that do
>> pointer arithmetic in address conversions. There will be no change in
>> compiled code with KASAN disabled since the compiler will optimize the
>> __tag_reset() out.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
>> ---
>> Changelog v4:
>> - Simplify page_to_virt() by removing pointless casts.
>> - Remove change in __is_canonical_address() because it's taken care of
>>   in a later patch due to a LAM compatible definition of canonical.
>> 
>>  arch/x86/include/asm/page.h    | 14 +++++++++++---
>>  arch/x86/include/asm/page_64.h |  2 +-
>>  arch/x86/mm/physaddr.c         |  1 +
>>  3 files changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
>> index 9265f2fca99a..15c95e96fd15 100644
>> --- a/arch/x86/include/asm/page.h
>> +++ b/arch/x86/include/asm/page.h
>> @@ -7,6 +7,7 @@
>>  #ifdef __KERNEL__
>>  
>>  #include <asm/page_types.h>
>> +#include <asm/kasan.h>
>>  
>>  #ifdef CONFIG_X86_64
>>  #include <asm/page_64.h>
>> @@ -41,7 +42,7 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
>>  #define __pa(x)		__phys_addr((unsigned long)(x))
>>  #endif
>>  
>> -#define __pa_nodebug(x)	__phys_addr_nodebug((unsigned long)(x))
>> +#define __pa_nodebug(x)	__phys_addr_nodebug((unsigned long)(__tag_reset(x)))
>
>Why not reset the tag inside __phys_addr_nodebug() and __phys_addr()?

Right, this should be one less line in the changelog and no behavior changes.
I'll fix it.

>
>>  /* __pa_symbol should be used for C visible symbols.
>>     This seems to be the official gcc blessed way to do such arithmetic. */
>>  /*
>> @@ -65,9 +66,16 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
>>   * virt_to_page(kaddr) returns a valid pointer if and only if
>>   * virt_addr_valid(kaddr) returns true.
>>   */
>> -#define virt_to_page(kaddr)	pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
>> +
>> +#ifdef CONFIG_KASAN_SW_TAGS
>> +#define page_to_virt(x) ({							\
>> +	void *__addr = __va(page_to_pfn((struct page *)x) << PAGE_SHIFT);	\
>> +	__tag_set(__addr, page_kasan_tag(x));					\
>> +})
>> +#endif
>> +#define virt_to_page(kaddr)	pfn_to_page(__pa((void *)__tag_reset(kaddr)) >> PAGE_SHIFT)
>
>then virt_to_page() will remain the same, no?

Oh, yes, that is redundant with __pa() resetting the tag. Thanks!

>
>>  extern bool __virt_addr_valid(unsigned long kaddr);
>> -#define virt_addr_valid(kaddr)	__virt_addr_valid((unsigned long) (kaddr))
>> +#define virt_addr_valid(kaddr)	__virt_addr_valid((unsigned long)(__tag_reset(kaddr)))
>
>The same here, I think tag_reset() should be inside __virt_addr_valid()

Sure, that does sound better.

>  
>>  static __always_inline void *pfn_to_kaddr(unsigned long pfn)
>>  {
>> diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
>> index 015d23f3e01f..de68ac40dba2 100644
>> --- a/arch/x86/include/asm/page_64.h
>> +++ b/arch/x86/include/asm/page_64.h
>> @@ -33,7 +33,7 @@ static __always_inline unsigned long __phys_addr_nodebug(unsigned long x)
>>  extern unsigned long __phys_addr(unsigned long);
>>  extern unsigned long __phys_addr_symbol(unsigned long);
>>  #else
>> -#define __phys_addr(x)		__phys_addr_nodebug(x)
>> +#define __phys_addr(x)		__phys_addr_nodebug(__tag_reset(x))
>>  #define __phys_addr_symbol(x) \
>>  	((unsigned long)(x) - __START_KERNEL_map + phys_base)
>>  #endif
>> diff --git a/arch/x86/mm/physaddr.c b/arch/x86/mm/physaddr.c
>> index fc3f3d3e2ef2..7f2b11308245 100644
>> --- a/arch/x86/mm/physaddr.c
>> +++ b/arch/x86/mm/physaddr.c
>> @@ -14,6 +14,7 @@
>>  #ifdef CONFIG_DEBUG_VIRTUAL
>>  unsigned long __phys_addr(unsigned long x)
>>  {
>> +	x = __tag_reset(x);
>>  	unsigned long y = x - __START_KERNEL_map;
>>  
>>  	/* use the carry flag to determine if x was < __START_KERNEL_map */
>> -- 
>> 2.50.1
>> 
>
>-- 
>Sincerely yours,
>Mike.

-- 
Kind regards
Maciej Wieczór-Retman