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().
--
Kind regards
Maciej Wieczór-Retman
