__builtin_constant_p() does not work in deep inline functions

[kees]

The hardened usercopy whitelisting (CONFIG_HARDENED_USERCOPY=y) depends on constant-sized arguments to copy_to_user()/copy_from_user() to be implicitly whitelisted. Without this, there is both a performance hit (for doing dynamic checking when none is needed) and failures (when an implicit whitelist is used with static sizes). For example, on x86, this happens under clang-5.0 but not gcc:

[ 1.628046] ------------[ cut here ]------------
[ 1.628524] Bad or missing usercopy whitelist? Kernel memory exposure attempt detected from SLUB object 'task_struct' (offset 1728, size 8)!
[ 1.629772] WARNING: CPU: 3 PID: 1208 at mm/usercopy.c:81 usercopy_warn+0x96/0xa0
[ 1.630514] Modules linked in:
[ 1.630519] CPU: 3 PID: 1208 Comm: sh Not tainted 4.16.0-rc5+ #64
[ 1.630520] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[ 1.630522] RIP: 0010:usercopy_warn+0x96/0xa0
[ 1.630523] RSP: 0018:ffff9c1781a73ce8 EFLAGS: 00010286
[ 1.630526] RAX: 947d865a0fb2e100 RBX: ffffffff96310daf RCX: ffffffff9665cdd0
[ 1.630527] RDX: ffffffff94edcfb7 RSI: ffffffff9665cd78 RDI: ffffffff94edcff8
[ 1.630528] RBP: ffff9c1781a73cf0 R08: 0000000000000000 R09: 0000000000000000
[ 1.630529] R10: 0000000000000002 R11: 0000000000000000 R12: ffff966a6bf6b3c8
[ 1.630530] R13: 0000000000000000 R14: 0000000000000008 R15: 0000000000000001
[ 1.630532] FS: 00007fc896687700(0000) GS:ffff966a7fd80000(0000) knlGS:0000000000000000
[ 1.630533] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1.630534] CR2: 00007ffc730a5d1c CR3: 000000042a81c001 CR4: 00000000001606e0
[ 1.630537] Call Trace:
[ 1.630541] __check_object_size+0xc3/0x1c0
[ 1.646723] do_signal+0x48e/0x5f0
[ 1.647127] prepare_exit_to_usermode+0xeb/0x170
[ 1.647742] syscall_return_slowpath+0x5e/0x2b0
[ 1.648240] ? syscall_trace_enter+0x15d/0x350
[ 1.648730] entry_SYSCALL_64_after_hwframe+0x42/0xb7
[ 1.649305] RIP: 0033:0x7fc89618012a
[ 1.649694] RSP: 002b:00007ffe56f8ec58 EFLAGS: 00000246 ORIG_RAX: 000000000000003d
[ 1.650522] RAX: 00000000000004bf RBX: 0000000000000001 RCX: 00007fc89618012a
[ 1.651294] RDX: 0000000000000000 RSI: 00007ffe56f8ec7c RDI: 00000000ffffffff
[ 1.652067] RBP: 0000563ebd2f63e0 R08: 0000000000000000 R09: 00007fc896687700
[ 1.652827] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[ 1.653586] R13: 00007ffe56f8ec7c R14: 00007ffe56f90fb3 R15: 0000000000000000
[ 1.654361] Code: 96 4c 0f 44 c0 4c 0f 44 c8 48 c7 c3 af 0d 31 96 48 0f 44 d8 48 c7 c7 31 0d 31 96 31 c0 41 52 41 53 53 e8 4e 2b e5 ff 48 83 c4 18 <0f> 0b 5b 5d c3 0f 1f 44 00 00 0f 1f 44 00 00 55 48 89 e5 53 4d
[ 1.656441] ---[ end trace aa26781ca73156e1 ]---

This is from arch/x86/kernel/signal.c do_signal+0x48e/0x5f0:
copy_user_generic at arch/x86/include/asm/uaccess_64.h:37
(inlined by) raw_copy_to_user at arch/x86/include/asm/uaccess_64.h:112
(inlined by) __copy_to_user at include/linux/uaccess.h:105
(inlined by) __setup_rt_frame at arch/x86/kernel/signal.c:493
(inlined by) setup_rt_frame at arch/x86/kernel/signal.c:699
(inlined by) handle_signal at arch/x86/kernel/signal.c:743
(inlined by) do_signal at arch/x86/kernel/signal.c:811

specifically __setup_rt_frame():

    err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));

Last argument is sizeof(*set), which should be true for __builtin_constant_p() in __copy_to_user():

    static __always_inline unsigned long
    __copy_to_user(void __user *to, const void *from, unsigned long n)
    {
            might_fault();
            kasan_check_read(from, n);
            check_object_size(from, n, true);
            return raw_copy_to_user(to, from, n);
    }

    static __always_inline void check_object_size(const void *ptr, unsigned long n,
                                                  bool to_user)
    {
            if (!__builtin_constant_p(n))
                    __check_object_size(ptr, n, to_user);
    }

check_object_size()'s n is __copy_to_user()'s n is __setup_rt_frame()'s sizeof(*set).

[gburgessiv]

Is needing __builtin_constant_p() to work through inlining a common thing? Or is there just an isolated place or two where it matters?

I ask because, while it's pretty terrible, you can make this work on a case-by-case basis with some clang-specific code:

    static __always_inline unsigned long
    __copy_to_user(void __user *to, const void *from, unsigned long n)
    {
            might_fault();
            kasan_check_read(from, n);
            check_object_size(from, n, true);
            return raw_copy_to_user(to, from, n);
    }

    static __always_inline void check_object_size(const void *ptr, unsigned long n,
                                                  bool to_user)
    {
            if (!__builtin_constant_p(n))
                    __check_object_size(ptr, n, to_user);
    }

    #ifdef __clang__
    #define enable_if_trivially_constant(x) enable_if((x) || !(x), "")
    static __always_inline __attribute__((overloadable, enable_if_trivially_constant(n)))
    unsigned long
    __copy_to_user(void __user *to, const void *from, unsigned long n)
    {
            might_fault();
            kasan_check_read(from, n);
            // Note the lack of __check_object_size.
            return raw_copy_to_user(to, from, n);
    }
    #endif

...I'd like to note, however, that this is forcing the decision of "should I call check_object_size" to be made during overload resolution. So, if anything more than trivial constant folding is required for n to be a constant, this won't work.

[kees]

I'm not sure how widely this assumption is used, but it's fairly central to a number of optimizations in usercopy code (not just the security hardening pieces)...

[nickdesaulniers]

https://llvm.org/pr4898

[nickdesaulniers]

https://lists.llvm.org/pipermail/llvm-dev/2018-April/122554.html

[nickdesaulniers]

https://reviews.llvm.org/D4276

[nickdesaulniers]

@jyknight is there anything we need to communicate to or patch in the upstream kernel?

[dileks]

CONFIG_HARDENED_USERCOPY=n is a workaround to have a bootable Linux v4.18-rc7 here on Debian/testing AMD64.

[kees]

Here's another recent case, which miscompiles (for arm64) due to inline vs __builtin_constant_p():

net/core/filter.c:7292: undefined reference to `__compiletime_assert_7292'

net/core/filter.c:7292:
BUILD_BUG_ON(hweight_long(SK_FL_PROTO_MASK) != BITS_PER_BYTE);

include/net/sock.h:
#define SK_FL_PROTO_MASK 0x0000ff00

include/linux/bitops.h:
static __always_inline unsigned long hweight_long(unsigned long w)
{
return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
}

include/asm-generic/bitops/const_hweight.h:
#define hweight64(w) (__builtin_constant_p(w) ? __const_hweight64(w) : __arch_hweight64(w))

"w" passed into hweight_long() loses its __builtin_constant-ness, and the BUILD_BUG_ON starts always evaluating with a full function call instead of as a constant expression.

[kees]

workaround:

-static __always_inline unsigned long hweight_long(unsigned long w)
-{
-       return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
-}
+#define hweight_long(w) (sizeof(w) == 4 ? hweight32(w) : hweight64(w))

[kees]

FWIW, the suggested Clang patch does not actually solve this issue for me. :(
(Though in looking, I think it's because the linked patch is only for LLVM, and maybe something is needed for Clang too?)

[dileks]

@kees Your workaround helped me to fix this in Linux v4.19-rc1...

+ ld -m elf_x86_64 -z max-page-size=0x200000 --emit-relocs --build-id -X -o .tmp_vmlinux1 -T ./arch/x86/kernel/vmlinux.lds --whole-archive built-in.a --no-whole-archive --start-group lib/lib.a arch/x86/lib/lib.a --end-group
ld: net/core/filter.o: in function `sk_reuseport_convert_ctx_access':
/home/sdi/src/linux-kernel/linux/net/core/filter.c:7284: undefined reference to `__compiletime_assert_7284'

[nickdesaulniers]

"w" passed into hweight_long() loses its __builtin_constant-ness, and the BUILD_BUG_ON starts always evaluating with a full function call instead of as a constant expression.

Does marking w const help?

Edit: update: looks like no...

[nickdesaulniers]

It seems that even in very basic cases of one level function call, clang and gcc disagree on __builtin_constant_p: https://godbolt.org/z/g_iqwh.

[dileks]

The workaround of Kees is no more needed with the patch "bpf: fix build error with clang" pending in bpf Git.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/patch/?id=3f6e138d41ddff196f452993528cfe75762ede0f

[bwendling]

Attaching two patch files if anyone would like to tests them out. (The llvm patch is from https://reviews.llvm.org/D4276)

bcp-clang.patch.txt
bcp-llvm.patch.txt

[kees]

These patches appear to solve at least the net/core/filter.c case! I'll need to double-check the runtime effects with HARDENED_USERCOPY on tuesday. (Note that I used my own forward-port of the LLVM patch based on D4276 -- I haven't compared that to the bcp-llvm.patch.txt above.)

[dileks]

@gwelymernans Your patches are against LLVM/Clang SVN revision 341155. Can you provide them against release_70 Git branch? So, I would like to test the case CONFIG_HARDENED_USERCOPY=y.

[kees]

New warning, which appears to be doing the wrong thing with "const u32 *", thinking it's a compile-time constant, when it's not:

drivers/pinctrl/pinctrl-rockchip.c:2489:26: warning: multiple unsequenced modifications to 'list' [-Wunsequenced]
                num = be32_to_cpu(*list++);
                                       ^~
./include/linux/byteorder/generic.h:95:21: note: expanded from macro 'be32_to_cpu'
#define be32_to_cpu __be32_to_cpu
                    ^
./include/uapi/linux/byteorder/little_endian.h:40:59: note: expanded from macro '__be32_to_cpu'
#define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
                                                          ^
./include/uapi/linux/swab.h:118:21: note: expanded from macro '__swab32'
        ___constant_swab32(x) :                 \
                           ^
./include/uapi/linux/swab.h:18:12: note: expanded from macro '___constant_swab32'
        (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
                  ^
drivers/pinctrl/pinctrl-rockchip.c:2494:52: warning: multiple unsequenced modifications to 'list' [-Wunsequenced]
                grp->pins[j] = bank->pin_base + be32_to_cpu(*list++);
                                                                 ^~

[dileks]

0001-Backport-bcp-llvm.patch-to-release_70-Git-branch.patch.txt
0001-Backport-bcp-clang.patch-to-release_70-Git-branch.patch.txt

I tried to backport the patches myself and have 8 failures when running ninja check-clang:

******************** TEST 'Clang :: Analysis/builtin-functions.cpp' FAILED ********************
******************** TEST 'Clang :: CXX/dcl.dcl/dcl.spec/dcl.constexpr/p5.cpp' FAILED ********************
******************** TEST 'Clang :: CXX/drs/dr5xx.cpp' FAILED ********************
******************** TEST 'Clang :: Sema/builtins.c' FAILED ********************
******************** TEST 'Clang :: Sema/const-eval.c' FAILED ********************
******************** TEST 'Clang :: Sema/i-c-e.c' FAILED ********************
******************** TEST 'Clang :: SemaCXX/builtin-assume-aligned.cpp' FAILED ********************
******************** TEST 'Clang :: SemaCXX/constexpr-printing.cpp' FAILED ********************

[bwendling]

@dileks The patches are more on the hack side of things. Do any of the failures look like semantic issues? or are they just that the tests expect different output?

[dileks]

@gwelymernans You had a chance to look into my backported patches? Can you verify they are done correctly? Thanks.

[bwendling]

@dileks They look fine to me (at least they look similar to what I did). I'm not surprised that there are failures in the tests. (They were written with the original assumption in mind.) I also may have missed something important when I was hacking the front-end patch together. (I'm not as familiar with the front-end code as the middle and back end code.)

[kees]

I found this, too: https://reviews.llvm.org/D35190 I wonder if we need this as part this work?

[bwendling]

@kees Could you add the pinctrl-rockship.i file and the command line it used here?

[gburgessiv]

I found this, too: https://reviews.llvm.org/D35190 I wonder if we need this as part this work?

Probably not. That patch is more for "when we're evaluating a constexpr function call, __builtin_constant_p(x) should be true if x is also constexpr in the context of that call."

From clang's perspective, if __builtin_constant_p == "can this possibly be folded to a constant with some effort?" the behavior offered in that patch's description is probably a good thing, but I think that patch would be a nop from the kernel's POV. (unless Linux has grown a lot of constexpr-y C++>=11 code? :) )