| Message ID | a8332a2dc5b21bd8533ea38da258c093fb9f2fe2.1743772053.git.maciej.wieczor-retman@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,01/14] kasan: sw_tags: Use arithmetic shift for shadow computation | expand |
On 4/4/25 06:14, Maciej Wieczor-Retman wrote: > +#ifdef CONFIG_KASAN_SW_TAGS > +#define page_to_virt(x) ({ \ > + __typeof__(x) __page = x; \ > + void *__addr = __va(page_to_pfn((__typeof__(x))__tag_reset(__page)) << PAGE_SHIFT); \ > + (void *)__tag_set((const void *)__addr, page_kasan_tag(__page)); \ > +}) > +#endif Is this #ifdef needed? I thought there were stub versions of all of those tag functions. So it should be harmless to use this page_to_virt() implementation with or without KASAN. Right? I'm also confused by the implementation. This is one reason why I rather dislike macros. Why does this act like the type of 'x' is variable? Isn't it always a 'struct page *'? If so, then why all of the __typeof__()'s? Are struct page pointers _ever_ tagged? If they are, then doesn't page_to_pfn() need to handle untagging as well? If they aren't, then there's no reason to __tag_reset() in here. What was the thinking behind this cast: (const void *)__addr ? Are any of these casts _doing_ anything? I'm struggling to find anything wrong with: #define page_to_virt(x) ({ void *__addr = __va(page_to_pfn(__page) << PAGE_SHIFT); __tag_set(__addr, page_kasan_tag(x)) }) ... which made me look back at: static inline const void *__tag_set(const void *addr, u8 tag) from patch 3. I don't think the 'const' makes any sense on the return value here. Surely the memory pointed at by a tagged pointer doesn't need to be const. Why should the tag setting function be returning a const pointer? I can see why it would *take* a const pointer since it's not modifying the memory, but I don't see why it is returning one.
On 2025-04-04 at 09:42:55 -0700, Dave Hansen wrote: >On 4/4/25 06:14, Maciej Wieczor-Retman wrote: >> +#ifdef CONFIG_KASAN_SW_TAGS >> +#define page_to_virt(x) ({ \ >> + __typeof__(x) __page = x; \ >> + void *__addr = __va(page_to_pfn((__typeof__(x))__tag_reset(__page)) << PAGE_SHIFT); \ >> + (void *)__tag_set((const void *)__addr, page_kasan_tag(__page)); \ >> +}) >> +#endif > >Is this #ifdef needed? > >I thought there were stub versions of all of those tag functions. So it >should be harmless to use this page_to_virt() implementation with or >without KASAN. Right? > >I'm also confused by the implementation. This is one reason why I rather >dislike macros. Why does this act like the type of 'x' is variable? >Isn't it always a 'struct page *'? If so, then why all of the >__typeof__()'s? > >Are struct page pointers _ever_ tagged? If they are, then doesn't >page_to_pfn() need to handle untagging as well? If they aren't, then >there's no reason to __tag_reset() in here. > >What was the thinking behind this cast: > > (const void *)__addr > >? > >Are any of these casts _doing_ anything? I'm struggling to find anything >wrong with: > >#define page_to_virt(x) ({ > void *__addr = __va(page_to_pfn(__page) << PAGE_SHIFT); > __tag_set(__addr, page_kasan_tag(x)) >}) > >... which made me look back at: > > static inline const void *__tag_set(const void *addr, u8 tag) > >from patch 3. I don't think the 'const' makes any sense on the return >value here. Surely the memory pointed at by a tagged pointer doesn't >need to be const. Why should the tag setting function be returning a >const pointer? > >I can see why it would *take* a const pointer since it's not modifying >the memory, but I don't see why it is returning one. > Right, yes, both your page_to_virt() and removing the const from __tag_set() return seem to be working just fine. Thanks for pointing it out. With the macros I was trying to do what the arm64 implementation did assuming it had some significance that wasn't written down anywhere. I recall lack of the const thing was giving me a compilation error a while back but now it's gone. And I didn't think much of it since arm had the same thing. If Andrey Konovalov is reading this: is there a reason the __tag_set() on arm64 returns a const pointer? And the page_to_virt() does the __typeof__()?
diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h index 9265f2fca99a..e37f63b50728 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))) /* __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,17 @@ 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) ({ \ + __typeof__(x) __page = x; \ + void *__addr = __va(page_to_pfn((__typeof__(x))__tag_reset(__page)) << PAGE_SHIFT); \ + (void *)__tag_set((const void *)__addr, page_kasan_tag(__page)); \ +}) +#endif +#define virt_to_page(kaddr) pfn_to_page(__pa((void *)__tag_reset(kaddr)) >> PAGE_SHIFT) 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))) static __always_inline void *pfn_to_kaddr(unsigned long pfn) { @@ -81,7 +90,7 @@ static __always_inline u64 __canonical_address(u64 vaddr, u8 vaddr_bits) static __always_inline u64 __is_canonical_address(u64 vaddr, u8 vaddr_bits) { - return __canonical_address(vaddr, vaddr_bits) == vaddr; + return __canonical_address(vaddr, vaddr_bits) == __tag_reset(vaddr); } #endif /* __ASSEMBLER__ */ diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h index d3aab6f4e59a..44975a8ae665 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 */
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@intel.com> --- arch/x86/include/asm/page.h | 17 +++++++++++++---- arch/x86/include/asm/page_64.h | 2 +- arch/x86/mm/physaddr.c | 1 + 3 files changed, 15 insertions(+), 5 deletions(-)