On Wed, Oct 6, 2021 at 5:56 AM Kees Cook [off-list ref] wrote:

On Wed, Oct 06, 2021 at 05:22:06AM +0200, Jann Horn wrote:

quoted

On Wed, Oct 6, 2021 at 5:06 AM Kees Cook [off-list ref] wrote:

quoted

On Tue, Oct 05, 2021 at 06:47:17PM -0700, Andrew Morton wrote:

quoted

On Thu, 30 Sep 2021 15:27:00 -0700 Kees Cook [off-list ref] wrote:

quoted

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?

quoted

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.