Re: [PATCH v3 11/14] x86: Handle int3 for inline KASAN reports

[Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>]

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