Re: [PATCH v3 4/8] slab: Add __alloc_size attributes for better bounds checking

From: Jann Horn <jannh@google.com>
To: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Christoph Lameter <cl@linux.com>,
	Pekka Enberg <penberg@kernel.org>,
	David Rientjes <rientjes@google.com>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Andy Whitcroft <apw@canonical.com>,
	Dennis Zhou <dennis@kernel.org>,
	Dwaipayan Ray <dwaipayanray1@gmail.com>,
	Joe Perches <joe@perches.com>,
	Lukas Bulwahn <lukas.bulwahn@gmail.com>,
	Miguel Ojeda <ojeda@kernel.org>,
	Nathan Chancellor <nathan@kernel.org>, Tejun Heo <tj@kernel.org>,
	Daniel Micay <danielmicay@gmail.com>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Michal Marek <michal.lkml@markovi.net>,
	clang-built-linux@googlegroups.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH v3 4/8] slab: Add __alloc_size attributes for better bounds checking
Date: Wed, 6 Oct 2021 06:52:21 +0200	[thread overview]
Message-ID: <CAG48ez30MS2sNQX1Sb-MDF7SbLEbD8p0ncOvQteyj38fygG8bQ@mail.gmail.com> (raw)
In-Reply-To: <202110052056.F09CD8A@keescook>

On Wed, Oct 6, 2021 at 5:56 AM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Oct 06, 2021 at 05:22:06AM +0200, Jann Horn wrote:
> > On Wed, Oct 6, 2021 at 5:06 AM Kees Cook <keescook@chromium.org> wrote:
> > > On Tue, Oct 05, 2021 at 06:47:17PM -0700, Andrew Morton wrote:
> > > > On Thu, 30 Sep 2021 15:27:00 -0700 Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > > As already done in GrapheneOS, add the __alloc_size attribute for regular
> > > > > kmalloc interfaces, to provide additional hinting for better bounds
> > > > > checking, assisting CONFIG_FORTIFY_SOURCE and other compiler
> > > > > optimizations.
> > > >
> > > > x86_64 allmodconfig:
> > >
> > > What compiler and version?
> > >
> > > >
> > > > In file included from ./arch/x86/include/asm/preempt.h:7,
> > > >                  from ./include/linux/preempt.h:78,
> > > >                  from ./include/linux/spinlock.h:55,
> > > >                  from ./include/linux/mmzone.h:8,
> > > >                  from ./include/linux/gfp.h:6,
> > > >                  from ./include/linux/mm.h:10,
> > > >                  from ./include/linux/mman.h:5,
> > > >                  from lib/test_kasan_module.c:10:
> > > > In function 'check_copy_size',
> > > >     inlined from 'copy_user_test' at ./include/linux/uaccess.h:191:6:
> > > > ./include/linux/thread_info.h:213:4: error: call to '__bad_copy_to' declared with attribute error: copy destination size is too small
> > > >   213 |    __bad_copy_to();
> > > >       |    ^~~~~~~~~~~~~~~
> > > > In function 'check_copy_size',
> > > >     inlined from 'copy_user_test' at ./include/linux/uaccess.h:199:6:
> > > > ./include/linux/thread_info.h:211:4: error: call to '__bad_copy_from' declared with attribute error: copy source size is too small
> > > >   211 |    __bad_copy_from();
> > > >       |    ^~~~~~~~~~~~~~~~~
> > > > make[1]: *** [lib/test_kasan_module.o] Error 1
> > > > make: *** [lib] Error 2
> > >
> > > Hah, yes, it caught an intentionally bad copy. This may bypass the
> > > check, as I've had to do in LKDTM before. I will test...
> > >
> > > diff --git a/lib/test_kasan_module.c b/lib/test_kasan_module.c
> > > index 7ebf433edef3..9fb2fb2937da 100644
> > > --- a/lib/test_kasan_module.c
> > > +++ b/lib/test_kasan_module.c
> > > @@ -19,7 +19,12 @@ static noinline void __init copy_user_test(void)
> > >  {
> > >         char *kmem;
> > >         char __user *usermem;
> > > -       size_t size = 128 - KASAN_GRANULE_SIZE;
> > > +       /*
> > > +        * This is marked volatile to avoid __alloc_size()
> > > +        * noticing the intentionally out-of-bounds copys
> > > +        * being done on the allocation.
> > > +        */
> > > +       volatile size_t size = 128 - KASAN_GRANULE_SIZE;
> >
> > Maybe OPTIMIZER_HIDE_VAR()? The normal version of that abuses an empty
> > asm statement to hide the value from the compiler.
>
> Oh! I hadn't seen that before. Is that better than volatile in this
> case?

It forces the compiler to assume an arbitrary modification of the
value, but doesn't force allocation of a stack slot like "volatile"
does, so it probably generates nicer code? Not that it really matters here...

It also has the difference that you can explicitly specify where the
compiler's analysis should cut off, instead of just doing it on every
access to a variable. But I guess maybe in this case, that's an
argument in favor of "volatile"? I don't know... I guess maybe
"volatile" does make sense here.