| Message ID | 012c84049b853d6853a7d6c887ce0c2323bcd80a.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: > When a tag mismatch happens in inline software tag-based KASAN on x86 an > int3 instruction is executed and needs proper handling. Does this mean "inline software"? Or "inline" functions? I'm not quite parsing that. I think it needs some more background. > Call kasan_report() from the int3 handler and pass down the proper > information from registers - RDI should contain the problematic address > and RAX other metadata. > > Also early return from the int3 selftest if inline KASAN is enabled > since it will cause a kernel panic otherwise. ... > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index bf82c6f7d690..ba277a25b57f 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -1979,6 +1979,9 @@ static noinline void __init int3_selftest(void) > }; > unsigned int val = 0; > > + if (IS_ENABLED(CONFIG_KASAN_INLINE)) > + return; Comments, please. This is a total non sequitur otherwise. > BUG_ON(register_die_notifier(&int3_exception_nb)); > > /* > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index 9f88b8a78e50..32c81fc2d439 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c ... > @@ -849,6 +850,51 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection) > cond_local_irq_disable(regs); > } > > +#ifdef CONFIG_KASAN_SW_TAGS > + > +#define KASAN_RAX_RECOVER 0x20 > +#define KASAN_RAX_WRITE 0x10 > +#define KASAN_RAX_SIZE_MASK 0x0f > +#define KASAN_RAX_SIZE(rax) (1 << ((rax) & KASAN_RAX_SIZE_MASK)) This ABI _looks_ like it was conjured out out of thin air. I assume it's coming from the compiler. Any pointers to that ABI definition in or out of the kernel would be appreciated. > +static bool kasan_handler(struct pt_regs *regs) > +{ > + int metadata = regs->ax; > + u64 addr = regs->di; > + u64 pc = regs->ip; > + bool recover = metadata & KASAN_RAX_RECOVER; > + bool write = metadata & KASAN_RAX_WRITE; > + size_t size = KASAN_RAX_SIZE(metadata); "metadata" is exactly the same length as "regs->ax", so it seems a little silly. Also, please use vertical alignment as a tool to make code more readable. Isn't this much more readable? bool recover = regs->ax & KASAN_RAX_RECOVER; bool write = regs->ax & KASAN_RAX_WRITE; size_t size = KASAN_RAX_SIZE(regs->ax); u64 addr = regs->di; u64 pc = regs->ip; > + if (!IS_ENABLED(CONFIG_KASAN_INLINE)) > + return false; > + > + if (user_mode(regs)) > + return false; > + > + kasan_report((void *)addr, size, write, pc); > + > + /* > + * The instrumentation allows to control whether we can proceed after > + * a crash was detected. This is done by passing the -recover flag to > + * the compiler. Disabling recovery allows to generate more compact > + * code. > + * > + * Unfortunately disabling recovery doesn't work for the kernel right > + * now. KASAN reporting is disabled in some contexts (for example when > + * the allocator accesses slab object metadata; this is controlled by > + * current->kasan_depth). All these accesses are detected by the tool, > + * even though the reports for them are not printed. > + * > + * This is something that might be fixed at some point in the future. > + */ Can we please find a way to do this that doesn't copy and paste a rather verbose comment? What if we passed 'recover' into kasan_report() and had it do the die()? > + if (!recover) > + die("Oops - KASAN", regs, 0); > + return true; > +} > + > +#endif > + > static bool do_int3(struct pt_regs *regs) > { > int res; > @@ -863,6 +909,12 @@ static bool do_int3(struct pt_regs *regs) > if (kprobe_int3_handler(regs)) > return true; > #endif > + > +#ifdef CONFIG_KASAN_SW_TAGS > + if (kasan_handler(regs)) > + return true; > +#endif I won't get _too_ grumbly about ti since there's another culprit right above, but the "no #fidefs in .c files" rule still applies. The right way to do this is with a stub kasan_handler() in a header with the #ifdef in the header. Actually, ditto on the kasan_handler() #ifdef. I suspect it can go away too and be replaced with a IS_ENABLED(CONFIG_KASAN_SW_TAGS) check.
On 2025-04-04 at 10:55:09 -0700, Dave Hansen wrote: >On 4/4/25 06:14, Maciej Wieczor-Retman wrote: >> When a tag mismatch happens in inline software tag-based KASAN on x86 an >> int3 instruction is executed and needs proper handling. > >Does this mean "inline software"? Or "inline" functions? I'm not quite >parsing that. I think it needs some more background. Both software KASAN modes (generic and tag-based) have an inline and outline variant. So I was referring to the inline mode in software tag-based mode. I'm mentioning "software" since there is also the "hardware" mode. > >> Call kasan_report() from the int3 handler and pass down the proper >> information from registers - RDI should contain the problematic address >> and RAX other metadata. >> >> Also early return from the int3 selftest if inline KASAN is enabled >> since it will cause a kernel panic otherwise. >... >> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c >> index bf82c6f7d690..ba277a25b57f 100644 >> --- a/arch/x86/kernel/alternative.c >> +++ b/arch/x86/kernel/alternative.c >> @@ -1979,6 +1979,9 @@ static noinline void __init int3_selftest(void) >> }; >> unsigned int val = 0; >> >> + if (IS_ENABLED(CONFIG_KASAN_INLINE)) >> + return; > >Comments, please. This is a total non sequitur otherwise. Sure, will add. > >> BUG_ON(register_die_notifier(&int3_exception_nb)); >> >> /* >> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c >> index 9f88b8a78e50..32c81fc2d439 100644 >> --- a/arch/x86/kernel/traps.c >> +++ b/arch/x86/kernel/traps.c >... >> @@ -849,6 +850,51 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection) >> cond_local_irq_disable(regs); >> } >> >> +#ifdef CONFIG_KASAN_SW_TAGS >> + >> +#define KASAN_RAX_RECOVER 0x20 >> +#define KASAN_RAX_WRITE 0x10 >> +#define KASAN_RAX_SIZE_MASK 0x0f >> +#define KASAN_RAX_SIZE(rax) (1 << ((rax) & KASAN_RAX_SIZE_MASK)) > >This ABI _looks_ like it was conjured out out of thin air. I assume it's >coming from the compiler. Any pointers to that ABI definition in or out >of the kernel would be appreciated. I'll put a comment that it's related to compilare ABI and I'll add a link to the relevant compiler file in the patch message. > >> +static bool kasan_handler(struct pt_regs *regs) >> +{ >> + int metadata = regs->ax; >> + u64 addr = regs->di; >> + u64 pc = regs->ip; >> + bool recover = metadata & KASAN_RAX_RECOVER; >> + bool write = metadata & KASAN_RAX_WRITE; >> + size_t size = KASAN_RAX_SIZE(metadata); > >"metadata" is exactly the same length as "regs->ax", so it seems a >little silly. Also, please use vertical alignment as a tool to make code >more readable. Isn't this much more readable? > > bool recover = regs->ax & KASAN_RAX_RECOVER; > bool write = regs->ax & KASAN_RAX_WRITE; > size_t size = KASAN_RAX_SIZE(regs->ax); > u64 addr = regs->di; > u64 pc = regs->ip; > Thanks, I'll apply this. >> + if (!IS_ENABLED(CONFIG_KASAN_INLINE)) >> + return false; >> + >> + if (user_mode(regs)) >> + return false; >> + >> + kasan_report((void *)addr, size, write, pc); >> + >> + /* >> + * The instrumentation allows to control whether we can proceed after >> + * a crash was detected. This is done by passing the -recover flag to >> + * the compiler. Disabling recovery allows to generate more compact >> + * code. >> + * >> + * Unfortunately disabling recovery doesn't work for the kernel right >> + * now. KASAN reporting is disabled in some contexts (for example when >> + * the allocator accesses slab object metadata; this is controlled by >> + * current->kasan_depth). All these accesses are detected by the tool, >> + * even though the reports for them are not printed. >> + * >> + * This is something that might be fixed at some point in the future. >> + */ > >Can we please find a way to do this that doesn't copy and paste a rather >verbose comment? > >What if we passed 'recover' into kasan_report() and had it do the die()? If that doesn't conflict somehow with how the kasan_report() is envisioned to work I think it's a good idea. Since risc-v will soon add this too I imagine? So it'd be copied in three places. > >> + if (!recover) >> + die("Oops - KASAN", regs, 0); >> + return true; >> +} >> + >> +#endif >> + >> static bool do_int3(struct pt_regs *regs) >> { >> int res; >> @@ -863,6 +909,12 @@ static bool do_int3(struct pt_regs *regs) >> if (kprobe_int3_handler(regs)) >> return true; >> #endif >> + >> +#ifdef CONFIG_KASAN_SW_TAGS >> + if (kasan_handler(regs)) >> + return true; >> +#endif >I won't get _too_ grumbly about ti since there's another culprit right >above, but the "no #fidefs in .c files" rule still applies. The right >way to do this is with a stub kasan_handler() in a header with the >#ifdef in the header. > >Actually, ditto on the kasan_handler() #ifdef. I suspect it can go away >too and be replaced with a IS_ENABLED(CONFIG_KASAN_SW_TAGS) check. Okay, thanks for pointing it out, I'll add the stub and IS_ENABLED().
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index bf82c6f7d690..ba277a25b57f 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -1979,6 +1979,9 @@ static noinline void __init int3_selftest(void) }; unsigned int val = 0; + if (IS_ENABLED(CONFIG_KASAN_INLINE)) + return; + BUG_ON(register_die_notifier(&int3_exception_nb)); /* diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 9f88b8a78e50..32c81fc2d439 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -16,6 +16,7 @@ #include <linux/interrupt.h> #include <linux/kallsyms.h> #include <linux/kmsan.h> +#include <linux/kasan.h> #include <linux/spinlock.h> #include <linux/kprobes.h> #include <linux/uaccess.h> @@ -849,6 +850,51 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection) cond_local_irq_disable(regs); } +#ifdef CONFIG_KASAN_SW_TAGS + +#define KASAN_RAX_RECOVER 0x20 +#define KASAN_RAX_WRITE 0x10 +#define KASAN_RAX_SIZE_MASK 0x0f +#define KASAN_RAX_SIZE(rax) (1 << ((rax) & KASAN_RAX_SIZE_MASK)) + +static bool kasan_handler(struct pt_regs *regs) +{ + int metadata = regs->ax; + u64 addr = regs->di; + u64 pc = regs->ip; + bool recover = metadata & KASAN_RAX_RECOVER; + bool write = metadata & KASAN_RAX_WRITE; + size_t size = KASAN_RAX_SIZE(metadata); + + if (!IS_ENABLED(CONFIG_KASAN_INLINE)) + return false; + + if (user_mode(regs)) + return false; + + kasan_report((void *)addr, size, write, pc); + + /* + * The instrumentation allows to control whether we can proceed after + * a crash was detected. This is done by passing the -recover flag to + * the compiler. Disabling recovery allows to generate more compact + * code. + * + * Unfortunately disabling recovery doesn't work for the kernel right + * now. KASAN reporting is disabled in some contexts (for example when + * the allocator accesses slab object metadata; this is controlled by + * current->kasan_depth). All these accesses are detected by the tool, + * even though the reports for them are not printed. + * + * This is something that might be fixed at some point in the future. + */ + if (!recover) + die("Oops - KASAN", regs, 0); + return true; +} + +#endif + static bool do_int3(struct pt_regs *regs) { int res; @@ -863,6 +909,12 @@ static bool do_int3(struct pt_regs *regs) if (kprobe_int3_handler(regs)) return true; #endif + +#ifdef CONFIG_KASAN_SW_TAGS + if (kasan_handler(regs)) + return true; +#endif + res = notify_die(DIE_INT3, "int3", regs, 0, X86_TRAP_BP, SIGTRAP); return res == NOTIFY_STOP;
When a tag mismatch happens in inline software tag-based KASAN on x86 an int3 instruction is executed and needs proper handling. Call kasan_report() from the int3 handler and pass down the proper information from registers - RDI should contain the problematic address and RAX other metadata. Also early return from the int3 selftest if inline KASAN is enabled since it will cause a kernel panic otherwise. Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> --- Changelog v3: - Add this patch to the series. arch/x86/kernel/alternative.c | 3 ++ arch/x86/kernel/traps.c | 52 +++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+)