Re: [PATCH v2 01/14] kasan: sw_tags: Use arithmetic shift for shadow computation

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

On 2025-03-01 at 01:21:52 +0100, Andrey Konovalov wrote:
>On Fri, Feb 28, 2025 at 5:13 PM Maciej Wieczor-Retman
><maciej.wieczor-retman@intel.com> wrote:
>>
>> I was applying your other comments to the series and came up with something like
>> this. What do you think?
>>
>>         /*
>>          * With the default kasan_mem_to_shadow() algorithm, all addresses
>>          * returned by the memory-to-shadow mapping (even for bogus pointers)
>>          * must be within a certain displacement from KASAN_SHADOW_OFFSET.
>>          *
>>          * For Generic KASAN the displacement is unsigned so the mapping from zero
>>          * to the last kernel address needs checking.
>>          */
>>         if (IS_ENABLED(CONFIG_KASAN_GENERIC)) {
>>                 if (addr < KASAN_SHADOW_OFFSET ||
>>                     addr >= KASAN_SHADOW_OFFSET + max_shadow_size)
>>                         return;
>>         } else {
>>                 /*
>>                  * For the tag-based mode the compiler resets tags in addresses at
>>                  * the start of kasan_mem_to_shadow(). Because of this it's not
>>                  * necessary to check a mapping of the entire address space but only
>>                  * whether a range of [0xFF00000000000000 - 0xFFFFFFFFFFFFFFFF] is a
>>                  * valid memory-to-shadow mapping. On x86, tags are located in bits
>>                  * 62:57 so the range becomes [0x7E00000000000000 - 0xFFFFFFFFFFFFFFFF].
>>                  * The check below tries to exclude invalid addresses by
>>                  * checking spaces between [0x7E00000000000000 - 0x7FFFFFFFFFFFFFFF]
>>                  * (which are positive and will overflow the memory-to-shadow
>>                  * mapping) and [0xFE00000000000000 - 0xFFFFFFFFFFFFFFFF]
>>                  */
>>                  if (addr > KASAN_SHADOW_OFFSET ||
>>                      (addr < (u64)kasan_mem_to_shadow((void *)(0xFEUL << 56)) &&
>>                      addr > (u64)kasan_mem_to_shadow((void *)(~0UL >> 1))) ||
>>                      addr < (u64)kasan_mem_to_shadow((void *)(0x7EUL << 56)))
>>                         return;
>>         }
>>
>> The comment is a bit long and has a lot of hexes but maybe it's good to leave a
>> longer explanation so no one has to dig through the mailing archives to
>> understand the logic :b
>
>Explaining the logic sounds good to me!
>
>I think your patch is close to what would look good, but I think the
>parentheses in the long if condition look suspicious.
>
>Please check the attached diff (Gmail makes it hard to inline code): I
>fixed the parentheses (if I'm right about them being wrong), made the
>checks look uniform, added an arm-specific check, and reworked the
>comments (please check if they make sense).
>
>If the diff looks good to you, let's use that.
>
>It also would be great, if you could test this: add some code that
>dereferences various bad addresses and see if the extra KASAN message
>line gets printed during the GPF.

Sure, I'll do these tests :)

For now I found my code has some issue with inline mode so I'll first try to
track down what's wrong there.

But looking at the patch you sent I'm wondering - are we treating the arithmetic
in kasan_mem_to_shadow() as unsigned? You wrote that all the ranges will
overflow but I thought we're interpreting the arithmetic as signed - so only
positive addresses will overflow and negative addresses (with bit 63 set) will
only be more negative thus not causing an overflow. That was my assumption when
writing the previous checks - we need to check below the overflown range, above
the negative (not overflown) range, and between the two.

-- 
Kind regards
Maciej Wieczór-Retman