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

From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
To: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Samuel Holland <samuel.holland@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	<linux-riscv@lists.infradead.org>,
	Andrey Ryabinin <ryabinin.a.a@gmail.com>,
	Alexander Potapenko <glider@google.com>,
	"Dmitry Vyukov" <dvyukov@google.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	<kasan-dev@googlegroups.com>, <llvm@lists.linux.dev>,
	Catalin Marinas <catalin.marinas@arm.com>,
	<linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>,
	Alexandre Ghiti <alexghiti@rivosinc.com>,
	Will Deacon <will@kernel.org>,
	Evgenii Stepanov <eugenis@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 1/9] kasan: sw_tags: Use arithmetic shift for shadow computation
Date: Mon, 10 Feb 2025 16:22:41 +0100	[thread overview]
Message-ID: <e7t5yzfw3dq5stp5xjy5yclcx6ikne4vwz7d6w2ukfw2b7gr6t@oomoynf3b2jl> (raw)
In-Reply-To: <CA+fCnZeBEe3VWm=VfYvG-f4eh2jAFP-p4Xn4SLEeFCGTudVuEw@mail.gmail.com>

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;

?

-- 
Kind regards
Maciej Wieczór-Retman