linux-kernel - Re: [PATCH 01/15] kasan: Allocation enhancement for dense tag-based mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+fCnZfsT3jO96rewM3wZw7n4hHJ44wRDG8g_55NFS5VG34grg@mail.gmail.com>
Date: Thu, 6 Feb 2025 19:14:33 +0100
From: Andrey Konovalov <andreyknvl@...il.com>
To: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
Cc: luto@...nel.org, xin@...or.com, kirill.shutemov@...ux.intel.com, 
	palmer@...belt.com, tj@...nel.org, brgerst@...il.com, ardb@...nel.org, 
	dave.hansen@...ux.intel.com, jgross@...e.com, will@...nel.org, 
	akpm@...ux-foundation.org, arnd@...db.de, corbet@....net, dvyukov@...gle.com, 
	richard.weiyang@...il.com, ytcoode@...il.com, tglx@...utronix.de, 
	hpa@...or.com, seanjc@...gle.com, paul.walmsley@...ive.com, 
	aou@...s.berkeley.edu, justinstitt@...gle.com, jason.andryuk@....com, 
	glider@...gle.com, ubizjak@...il.com, jannh@...gle.com, bhe@...hat.com, 
	vincenzo.frascino@....com, rafael.j.wysocki@...el.com, 
	ndesaulniers@...gle.com, mingo@...hat.com, catalin.marinas@....com, 
	junichi.nomura@....com, nathan@...nel.org, ryabinin.a.a@...il.com, 
	dennis@...nel.org, bp@...en8.de, kevinloughlin@...gle.com, morbo@...gle.com, 
	dan.j.williams@...el.com, julian.stecklina@...erus-technology.de, 
	peterz@...radead.org, cl@...ux.com, kees@...nel.org, 
	kasan-dev@...glegroups.com, x86@...nel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-riscv@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, llvm@...ts.linux.dev, 
	linux-doc@...r.kernel.org
Subject: Re: [PATCH 01/15] kasan: Allocation enhancement for dense tag-based mode

On Thu, Feb 6, 2025 at 1:58 PM Maciej Wieczor-Retman
<maciej.wieczor-retman@...el.com> wrote:
>
> >Is there a reason these definitions are added to
> >include/linux/kasan.h? At least within this patch, they are only used
> >within mm/kasan, so let's keep them in mm/kasan/kasan.h.
>
> Parts of x86 arch use these later (minimal slab alignment, kasan shadow start
> address) so I thought it was convenient to already have it in place here?

AFAICT, KASAN_SHADOW_START only relies on KASAN_SHADOW_SCALE_SHIFT,
which is defined arch/x86/include/asm/kasan.h anyway.

And ARCH_SLAB_MINALIGN is defined in asm headers, so the definitions
from include/linux/kasan.h shouldn't be visible to it?

I think that we need to do is to define KASAN_GRANULE_SHIFT next to
KASAN_SHADOW_SCALE_SHIFT for x86 and then use it in mm/kasan/kasan.h
to define KASAN_GRANULE_SIZE for SW_TAGS. (Similarly as with arm64,
where ARCH_SLAB_MINALIGN depends on either KASAN_SHADOW_SCALE_SHIFT or
MTE_GRANULE_SIZE, both of which are defined in arm64 asm headers.)

Btw, I think ARCH_SLAB_MINALIGN needs to be defined in
include/asm/cache.h: at least all other architectures have it there.

> Since I'll be reordering patches I can just move these changes together.

Otherwise, if you need to expose something new in
include/linux/kasan.h, please do it together with the change that uses
it. Or you can even put it into a separate patch with an explanation
of why it's required - at least from the review perspective having
separate smaller patches is often better.

In general, if something doesn't need to get exposed to the rest of
the kernel, keep it in mm/kasan/kasan.h.

> >I think this should also depend on KASAN_OUTLINE: Clang/GCC aren't
> >aware of the dense mode.
>
> I wasn't sure I fully understood how inline/outline interacts with clang/gcc on
> x86 (especially that I think some parts are still missing in x86 clang for
> tag-based KASAN). So I understand that compiling with inline doesn't do
> anything? If so, is it not doing anything because of missing compiler code or
> something in the kernel?

With inline instrumentation, the compiler directly embeds the
instructions to calculate the shadow address and check the shadow
value. Since the compiler assumes that one shadow byte corresponds to
16 bytes of memory and not 32, the generated instructions won't be
compatible with the dense mode. With outline instrumentation, the
compiler just adds function calls and thus all the shadow calculations
are performed by the C code.

Or did the dense mode work for you with KASAN_INLINE enabled? I would
expect this not to work. Or maybe the inline instrumentation somehow
got auto-disabled...

> >Would it be possible to move this part to kasan_poison_last_granule()?
> >That functions seems to be serving a similar purpose but for the
> >Generic mode.
> >
> >It might also be cleaner to add a kasan_poison_first_granule() that
> >contains the if (addr64 % KASAN_SHADOW_SCALE_SIZE) check.
> ...
> sure, I'll try to move these checks to kasan_poison_first/last_granule.

For kasan_poison_last_granule(), I think the change makes sense. For
kasan_poison_first_granule(), please check whether it gives any
readability benefit - if kasan_poison() is the only caller, maybe
adding another function is not worth it.