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: <aJ2M_eKPvBluyLKJ@kernel.org>
Date: Thu, 14 Aug 2025 10:15:09 +0300
From: Mike Rapoport <rppt@...nel.org>
To: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
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

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()?

>  /* __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?

>  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()
  
>  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.