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

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>] [day] [month] [year] [list]

Message-Id: <20170731124128.461001-1-arnd@arndb.de>
Date:   Mon, 31 Jul 2017 11:50:38 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Alexander Viro <viro@...iv.linux.org.uk>
Cc:     Arnd Bergmann <arnd@...db.de>, Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Dmitry Safonov <dsafonov@...tuozzo.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH] [RFC] fs/binfmt_elf: work around bogus ubsan array-bounds warning

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@...db.de>
---
 fs/binfmt_elf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
 	}

-- 
2.9.0