| Message ID | 20170731095228.461151-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 31, 2017 at 2:52 AM, Arnd Bergmann <arnd@arndb.de> wrote: > 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. IIUC, __copy*() is allowed here because the kernel is operating on an already sanity checked pointer (i.e. a freshly kernel-allocated stack). I wouldn't expect swapping in copy*() to have noticeable performance here, though if there was, it would be a constant change (the ELF tables are a per-arch fixed size). -Kees
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; }
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(-)