diff mbox

[RFC] fs/binfmt_elf: work around bogus ubsan array-bounds warning

Message ID 20170731124128.461001-1-arnd@arndb.de
State New
Headers show

Commit Message

Arnd Bergmann July 31, 2017, 9:50 a.m. UTC 
I don't really understand this warning, but it does seem to get triggered
by the unusual pointer notation used in the x86_64 __copy_to_user()
optimization. Here, the source buffer for __copy_to_user() is the
constant expression '("x86_64")', which is seven bytes long and does
not trigger the optimized fast-path that handles buffers of 1, 2, 4, 8,
10 and 16 bytes.

I suspect this is another case of __builtin_constant_p() not doing
exactly what we expect it to do: The compiler first decides that it
knows the string length of the constant expression, but then
it does not eliminate the 'case 10' and 'case  16' portions of the
switch() statement before checking for a possible buffer overflow.

The warning we now get is:

In file included from include/linux/uaccess.h:13:0,
                 from include/linux/highmem.h:8,
                 from /home/arnd/arm-soc/fs/binfmt_elf.c:28:
fs/binfmt_elf.c: In function 'create_elf_tables':
arch/x86/include/asm/uaccess_64.h:143:20: error: array subscript is above array bounds [-Werror=array-bounds]
    __put_user_asm(4[(u16 *)src], 4 + (u16 __user *)dst,
                    ^
arch/x86/include/asm/uaccess.h:473:16: note: in definition of macro '__put_user_asm'
        : ltype(x), "m" (__m(addr)), "i" (errret), "0" (err))
                ^
arch/x86/include/asm/uaccess_64.h:143:20: error: array subscript is above array bounds [-Werror=array-bounds]
    __put_user_asm(4[(u16 *)src], 4 + (u16 __user *)dst,
                    ^
arch/x86/include/asm/uaccess.h:473:16: note: in definition of macro '__put_user_asm'
        : ltype(x), "m" (__m(addr)), "i" (errret), "0" (err))
                ^
arch/x86/include/asm/uaccess_64.h:154:20: error: array subscript is above array bounds [-Werror=array-bounds]
    __put_user_asm(1[(u64 *)src], 1 + (u64 __user *)dst,
                    ^
arch/x86/include/asm/uaccess.h:473:16: note: in definition of macro '__put_user_asm'
        : ltype(x), "m" (__m(addr)), "i" (errret), "0" (err))
                ^
arch/x86/include/asm/uaccess_64.h:154:20: error: array subscript is above array bounds [-Werror=array-bounds]
    __put_user_asm(1[(u64 *)src], 1 + (u64 __user *)dst,
                    ^
arch/x86/include/asm/uaccess.h:473:16: note: in definition of macro '__put_user_asm'
        : ltype(x), "m" (__m(addr)), "i" (errret), "0" (err))
                ^

As we know from other false-positive warnings that we get with UBSAN,
the sanitizers disable a couple of optimization steps in the compiler
that get in the way of sanitizing, but otherwise would let the compiler
figure out that it doesn't need to warn.

I considered turning off -Warray-bounds whenever UBSAN is enabled,
however, I got exactly two such warnings in the entire kernel with UBSAN,
and the other one turned out to be a real kernel stack overflow.

Using copy_to_user instead of __copy_to_user shuts up the warning here
and is harmless, but is otherwise a completely bogus change as
the function is still using a mix of __copy_to_user and copy_to_user.

I have not found out why create_elf_tables() uses the __copy_to_user
version in the first place, and the right answer might be that it
should simply use copy_to_user() and put_user() everywhere.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 fs/binfmt_elf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.9.0
diff mbox

Patch

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 879ff9c7ffd0..f4fe681855ec 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -195,7 +195,7 @@  create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 		size_t len = strlen(k_platform) + 1;
 
 		u_platform = (elf_addr_t __user *)STACK_ALLOC(p, len);
-		if (__copy_to_user(u_platform, k_platform, len))
+		if (copy_to_user(u_platform, k_platform, len))
 			return -EFAULT;
 	}