linux-kernel - Re: [PATCH v8 09/14] x86/mm: LAM compatible non-canonical definition

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: <aWpuuiuqZhkGRj2B@google.com>
Date: Fri, 16 Jan 2026 09:00:42 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
Cc: Andrey Ryabinin <ryabinin.a.a@...il.com>, Maciej Wieczor-Retman <m.wieczorretman@...me>, 
	Thomas Gleixner <tglx@...nel.org>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Alexander Potapenko <glider@...gle.com>, linux-kernel@...r.kernel.org, 
	Paolo Bonzini <pbonzini@...hat.com>, kvm <kvm@...r.kernel.org>
Subject: Re: [PATCH v8 09/14] x86/mm: LAM compatible non-canonical definition

On Fri, Jan 16, 2026, Maciej Wieczor-Retman wrote:
> On 2026-01-16 at 06:57:04 -0800, Sean Christopherson wrote:
> >On Fri, Jan 16, 2026, Andrey Ryabinin wrote:
> >> On 1/12/26 6:28 PM, Maciej Wieczor-Retman wrote:
> >> > diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
> >> > index bcf5cad3da36..b7940fa49e64 100644
> >> > --- a/arch/x86/include/asm/page.h
> >> > +++ b/arch/x86/include/asm/page.h
> >> > @@ -82,9 +82,22 @@ static __always_inline void *pfn_to_kaddr(unsigned long pfn)
> >> >  	return __va(pfn << PAGE_SHIFT);
> >> >  }
> >> >
> >> > +#ifdef CONFIG_KASAN_SW_TAGS
> >> > +#define CANONICAL_MASK(vaddr_bits) (BIT_ULL(63) | BIT_ULL((vaddr_bits) - 1))
> >>
> >> why is the choice of CANONICAL_MASK() gated at compile time? Shouldn’t this be a
> >> runtime decision based on whether LAM is enabled or not on the running system?
> 
> What would be appropriate for KVM? Instead of using #ifdefs checking
> if(cpu_feature_enabled(X86_FEATURE_LAM))?

None of the above.  Practically speaking, the kernel APIs simply can't automatically
handle the checks, because they are dependent on guest virtual CPU state, _and_
on the exact operation.  E.g. LAM doesn't apply to inputs to TLB invalidation
instructions like INVVPID and INVPCID.

By the time KVM invokes __is_canonical_address(), KVM has already done the necessary
LAM unmasking.  E.g. having KVM pass in a flag saying it doesn't need LAM masking
would be rather silly.

> >> > +#else
> >> > +#define CANONICAL_MASK(vaddr_bits) GENMASK_ULL(63, vaddr_bits)
> >> > +#endif
> >> > +
> >> > +/*
> >> > + * To make an address canonical either set or clear the bits defined by the
> >> > + * CANONICAL_MASK(). Clear the bits for userspace addresses if the top address
> >> > + * bit is a zero. Set the bits for kernel addresses if the top address bit is a
> >> > + * one.
> >> > + */
> >> >  static __always_inline u64 __canonical_address(u64 vaddr, u8 vaddr_bits)
> >>
> >> +Cc KVM
> >
> >Thanks!
> >
> >> This is used extensively in KVM code. As far as I can tell, it may be used to
> >> determine whether a guest virtual address is canonical or not.
> >
> >Yep, KVM uses this both to check canonical addresses and to force a canonical
> >address (Intel and AMD disagree on the MSR_IA32_SYSENTER_{EIP,ESP} semantics in
> >64-bit mode) for guest addresses.  This change will break KVM badly if KASAN_SW_TAGS=y.
> 
> Oh, thanks! That's good to know.
> 
> >
> >> case, the result should depend on whether LAM is enabled for the guest, not
> >> the host (and certainly not a host's compile-time option).
> >
> >Ya, KVM could roll its own versions, but IMO these super low level helpers should
> >do exactly what they say.  E.g. at a glance, I'm not sure pt_event_addr_filters_sync()
> >should be subjected to KASAN_SW_TAGS either.  If that's true, then AFAICT the
> >_only_ code that wants the LAM-aware behavior is copy_from_kernel_nofault_allowed(),
> >so maybe just handle it there?  Not sure that's a great long-term maintenance
> >story either though.
> 
> Yes, longterm it's probably best to just get it right in here.

As above, I don't think that's feasible, because the context of both the current
(virtual) CPU and the usage matters.  In other words, making __canonical_address()
itself LAM-aware feels wrong.

Actually, the kernel already has to deal with masking LAM bits for userspace
addresses, and this series needs to unmask kernel address in other flows that
effectively consume virtual addresses in software, so why not just do something
similar for copy_from_kernel_nofault_allowed()?

diff --git a/arch/x86/mm/maccess.c b/arch/x86/mm/maccess.c
index 42115ac079cf..0b3c96f8902a 100644
--- a/arch/x86/mm/maccess.c
+++ b/arch/x86/mm/maccess.c
@@ -33,7 +33,8 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
        if (!boot_cpu_data.x86_virt_bits)
                return true;
 
-       return __is_canonical_address(vaddr, boot_cpu_data.x86_virt_bits);
+       return __is_canonical_address(__tag_reset(vaddr),
+                                     boot_cpu_data.x86_virt_bits);
 }
 #else
 bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)