Re: [PATCH v2 1/9] kasan: sw_tags: Use arithmetic shift for shadow computation

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

On 2025-02-11 at 09:58:22 +0100, Maciej Wieczor-Retman wrote:
>On 2025-02-10 at 23:57:10 +0100, Andrey Konovalov wrote:
>>On Mon, Feb 10, 2025 at 4:53 PM Maciej Wieczor-Retman
>><maciej.wieczor-retman@intel.com> wrote:
>>>
>>> On 2025-02-10 at 16:22:41 +0100, Maciej Wieczor-Retman wrote:
>>> >On 2024-10-23 at 20:41:57 +0200, Andrey Konovalov wrote:
>>> >>On Tue, Oct 22, 2024 at 3:59 AM Samuel Holland
>>> >><samuel.holland@sifive.com> wrote:
>>> >...
>>> >>> +        * Software Tag-Based KASAN, the displacement is signed, so
>>> >>> +        * KASAN_SHADOW_OFFSET is the center of the range.
>>> >>>          */
>>> >>> -       if (addr < KASAN_SHADOW_OFFSET)
>>> >>> -               return;
>>> >>> +       if (IS_ENABLED(CONFIG_KASAN_GENERIC)) {
>>> >>> +               if (addr < KASAN_SHADOW_OFFSET ||
>>> >>> +                   addr >= KASAN_SHADOW_OFFSET + max_shadow_size)
>>> >>> +                       return;
>>> >>> +       } else {
>>> >>> +               if (addr < KASAN_SHADOW_OFFSET - max_shadow_size / 2 ||
>>> >>> +                   addr >= KASAN_SHADOW_OFFSET + max_shadow_size / 2)
>>> >>> +                       return;
>>> >>
>>> >>Hm, I might be wrong, but I think this check does not work.
>>> >>
>>> >>Let's say we have non-canonical address 0x4242424242424242 and number
>>> >>of VA bits is 48.
>>> >>
>>> >>Then:
>>> >>
>>> >>KASAN_SHADOW_OFFSET == 0xffff800000000000
>>> >>kasan_mem_to_shadow(0x4242424242424242) == 0x0423a42424242424
>>> >>max_shadow_size == 0x1000000000000000
>>> >>KASAN_SHADOW_OFFSET - max_shadow_size / 2 == 0xf7ff800000000000
>>> >>KASAN_SHADOW_OFFSET + max_shadow_size / 2 == 0x07ff800000000000 (overflows)
>>> >>
>>> >>0x0423a42424242424 is < than 0xf7ff800000000000, so the function will
>>> >>wrongly return.
>>> >
>>> >As I understand this check aims to figure out if the address landed in shadow
>>> >space and if it didn't we can return.
>>> >
>>> >Can't this above snippet be a simple:
>>> >
>>> >       if (!addr_in_shadow(addr))
>>> >               return;
>>> >
>>> >?
>>>
>>> Sorry, I think this wouldn't work. The tag also needs to be reset. Does this
>>> perhaps work for this problem?
>>>
>>>         if (!addr_in_shadow(kasan_reset_tag((void *)addr)))
>>>                 return;
>>
>>This wouldn't work as well.
>>
>>addr_in_shadow() checks whether an address belongs to the proper
>>shadow memory area. That area is the result of the memory-to-shadow
>>mapping applied to the range of proper kernel addresses.
>>
>>However, what we want to check in this function is whether the given
>>address can be the result of the memory-to-shadow mapping for some
>>memory address, including userspace addresses, non-canonical
>>addresses, etc. So essentially we need to check whether the given
>>address belongs to the area that is the result of the memory-to-shadow
>>mapping applied to the whole address space, not only to proper kernel
>>addresses.k
>
>Ah, okay, I get it. Would the old version
>
>       if (addr < KASAN_SHADOW_OFFSET)
>               return;
>
>work if the *addr* had kasan_reset_tag() around it? That would sort of re-unsign
>the address only for the purpose of the if().
>
>Also I was thinking about it because x86 even with address masking enabled keeps
>bit 63 set, so all kernel addresses will be negative in the signed
>kasan_mem_to_shadow(). That's great for simplifying the KASAN_SHADOW_OFFSET but
>it differs from the TBI and risc-v ideas where half of addresses are negative,
>hald positive. So the temporary re-unsigning could maybe make it simpler for x86
>and avoid adding separate cases or alternative kasan_non_canonical_hook()
>implementation.

Oh, nevermind, I see that this is more complicated than that. Sorry for the
spam, I'll do some better calculations what is mapped where when doing
kasan_mem_to_shadow() and maybe then I'll figure this out.

-- 
Kind regards
Maciej Wieczór-Retman