objtool warning in drivers/bluetooth/hci_vhci.o around vhci_coredump_hdr()

[nathanchance]

The kernel test robot reported an objtool warning with this configuration, which it pointed the finger at commit ab4e4380d4e1 ("Bluetooth: Add vhci devcoredump support") a while ago. Depending on the clang version, you will see one of two warnings:

drivers/bluetooth/hci_vhci.o: warning: objtool: .text: unexpected end of section

drivers/bluetooth/hci_vhci.o: warning: objtool: vhci_coredump_hdr() falls through to next function __cfi_vhci_open_timeout()

The body of vhci_coredump_hdr():

static void vhci_coredump_hdr(struct hci_dev *hdev, struct sk_buff *skb)
{
	char buf[80];

	snprintf(buf, sizeof(buf), "Controller Name: vhci_ctrl\n");
	skb_put_data(skb, buf, strlen(buf));

	snprintf(buf, sizeof(buf), "Firmware Version: vhci_fw\n");
	skb_put_data(skb, buf, strlen(buf));

	snprintf(buf, sizeof(buf), "Driver: vhci_drv\n");
	skb_put_data(skb, buf, strlen(buf));

	snprintf(buf, sizeof(buf), "Vendor: vhci\n");
	skb_put_data(skb, buf, strlen(buf));
}

I bisected the original introduction of this warning to llvm/llvm-project@1f88d80, which looks like a reasonable commit to blame.

My cvise attempt ended up not producing much, it was long and had overtly undefined behavior.

The diff of the IR from the parent of the problematic change shows that it completely eliminates the body of vhci_coredump_hdr(), transforming it simply into unreachable.

@@ -1103,107 +1096,8 @@ entry:
 
 ; Function Attrs: noredzone nounwind null_pointer_is_valid ssp
 define internal void @vhci_coredump_hdr(ptr nocapture readnone %hdev, ptr noundef %skb) #2 align 16 {
-if.end15.i.i:
-  %buf = alloca [80 x i8], align 16
-  call void @llvm.lifetime.start.p0(i64 80, ptr nonnull %buf) #15
-  call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 16 dereferenceable(28) %buf, ptr noundef nonnull align 1 dereferenceable(28) @.str.11, i64 28, i1 false)
-  %call18.i.i = call i64 @strnlen(ptr noundef nonnull %buf, i64 noundef 80) #14
-  %.not.i = icmp ugt i64 %call18.i.i, 80
-  br i1 %.not.i, label %do.end36.i.i, label %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i
-
-do.end36.i.i:                                     ; preds = %if.end15.i.i
-  %add.i.i = add i64 %call18.i.i, 1
-  tail call void @__fortify_panic(i8 noundef zeroext 2, i64 noundef 80, i64 noundef %add.i.i) #18
-  unreachable
-
-_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i: ; preds = %if.end15.i.i
-  %cmp2.not.i.not = icmp eq i64 %call18.i.i, 80
-  br i1 %cmp2.not.i.not, label %do.end15.i, label %_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit
-
-do.end15.i:                                       ; preds = %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i
-  %add.i = add nuw nsw i64 %call18.i.i, 1
-  tail call void @__fortify_panic(i8 noundef zeroext 4, i64 noundef 80, i64 noundef %add.i) #18
-  unreachable
-
-_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit: ; preds = %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i
-  %conv = trunc i64 %call18.i.i to i32
-  %call.i = tail call ptr @skb_put(ptr noundef %skb, i32 noundef %conv) #14
-  %0 = tail call i64 asm "", "=r,0,~{dirflag},~{fpsr},~{flags}"(i64 %call18.i.i) #17, !srcloc !17
-  call void @llvm.memcpy.p0.p0.i64(ptr align 1 %call.i, ptr nonnull align 16 %buf, i64 %0, i1 false) #15
-  call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 16 dereferenceable(27) %buf, ptr noundef nonnull align 1 dereferenceable(27) @.str.12, i64 27, i1 false)
-  %call18.i.i38 = call i64 @strnlen(ptr noundef nonnull %buf, i64 noundef 80) #14
-  %.not.i39 = icmp ugt i64 %call18.i.i38, 80
-  br i1 %.not.i39, label %do.end36.i.i42, label %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i45
-
-do.end36.i.i42:                                   ; preds = %_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit
-  %add.i.i41 = add i64 %call18.i.i38, 1
-  tail call void @__fortify_panic(i8 noundef zeroext 2, i64 noundef 80, i64 noundef %add.i.i41) #18
-  unreachable
-
-_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i45: ; preds = %_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit
-  %cmp2.not.i44.not = icmp eq i64 %call18.i.i38, 80
-  br i1 %cmp2.not.i44.not, label %do.end15.i47, label %_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit48
-
-do.end15.i47:                                     ; preds = %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i45
-  %add.i46 = add nuw nsw i64 %call18.i.i38, 1
-  tail call void @__fortify_panic(i8 noundef zeroext 4, i64 noundef 80, i64 noundef %add.i46) #18
-  unreachable
-
-_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit48: ; preds = %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i45
-  %conv10 = trunc i64 %call18.i.i38 to i32
-  %call.i90 = tail call ptr @skb_put(ptr noundef %skb, i32 noundef %conv10) #14
-  %1 = tail call i64 asm "", "=r,0,~{dirflag},~{fpsr},~{flags}"(i64 %call18.i.i38) #17, !srcloc !17
-  call void @llvm.memcpy.p0.p0.i64(ptr align 1 %call.i90, ptr nonnull align 16 %buf, i64 %1, i1 false) #15
-  call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 16 dereferenceable(18) %buf, ptr noundef nonnull align 1 dereferenceable(18) @.str.13, i64 18, i1 false)
-  %call18.i.i58 = call i64 @strnlen(ptr noundef nonnull %buf, i64 noundef 80) #14
-  %.not.i59 = icmp ugt i64 %call18.i.i58, 80
-  br i1 %.not.i59, label %do.end36.i.i62, label %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i65
-
-do.end36.i.i62:                                   ; preds = %_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit48
-  %add.i.i61 = add i64 %call18.i.i58, 1
-  tail call void @__fortify_panic(i8 noundef zeroext 2, i64 noundef 80, i64 noundef %add.i.i61) #18
+do.end36.i.i:
   unreachable
-
-_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i65: ; preds = %_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit48
-  %cmp2.not.i64.not = icmp eq i64 %call18.i.i58, 80
-  br i1 %cmp2.not.i64.not, label %do.end15.i67, label %_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit68
-
-do.end15.i67:                                     ; preds = %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i65
-  %add.i66 = add nuw nsw i64 %call18.i.i58, 1
-  tail call void @__fortify_panic(i8 noundef zeroext 4, i64 noundef 80, i64 noundef %add.i66) #18
-  unreachable
-
-_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit68: ; preds = %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i65
-  %conv17 = trunc i64 %call18.i.i58 to i32
-  %call.i111 = tail call ptr @skb_put(ptr noundef %skb, i32 noundef %conv17) #14
-  %2 = tail call i64 asm "", "=r,0,~{dirflag},~{fpsr},~{flags}"(i64 %call18.i.i58) #17, !srcloc !17
-  call void @llvm.memcpy.p0.p0.i64(ptr align 1 %call.i111, ptr nonnull align 16 %buf, i64 %2, i1 false) #15
-  call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 16 dereferenceable(14) %buf, ptr noundef nonnull align 1 dereferenceable(14) @.str.14, i64 14, i1 false)
-  %call18.i.i78 = call i64 @strnlen(ptr noundef nonnull %buf, i64 noundef 80) #14
-  %.not.i79 = icmp ugt i64 %call18.i.i78, 80
-  br i1 %.not.i79, label %do.end36.i.i82, label %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i85
-
-do.end36.i.i82:                                   ; preds = %_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit68
-  %add.i.i81 = add i64 %call18.i.i78, 1
-  tail call void @__fortify_panic(i8 noundef zeroext 2, i64 noundef 80, i64 noundef %add.i.i81) #18
-  unreachable
-
-_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i85: ; preds = %_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit68
-  %cmp2.not.i84.not = icmp eq i64 %call18.i.i78, 80
-  br i1 %cmp2.not.i84.not, label %do.end15.i87, label %_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit88
-
-do.end15.i87:                                     ; preds = %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i85
-  %add.i86 = add nuw nsw i64 %call18.i.i78, 1
-  tail call void @__fortify_panic(i8 noundef zeroext 4, i64 noundef 80, i64 noundef %add.i86) #18
-  unreachable
-
-_Z16__fortify_strlenPKcU25pass_dynamic_object_size1.exit88: ; preds = %_Z7strnlenPKcU25pass_dynamic_object_size1m.exit.i85
-  %conv24 = trunc i64 %call18.i.i78 to i32
-  %call.i132 = tail call ptr @skb_put(ptr noundef %skb, i32 noundef %conv24) #14
-  %3 = tail call i64 asm "", "=r,0,~{dirflag},~{fpsr},~{flags}"(i64 %call18.i.i78) #17, !srcloc !17
-  call void @llvm.memcpy.p0.p0.i64(ptr align 1 %call.i132, ptr nonnull align 16 %buf, i64 %3, i1 false) #15
-  call void @llvm.lifetime.end.p0(i64 80, ptr nonnull %buf) #15
-  ret void
 }
 
 ; Function Attrs: noredzone null_pointer_is_valid
@@ -1221,9 +1115,6 @@ declare dso_local i32 @hci_devcd_abort(ptr noundef) local_unnamed_addr #1
 ; Function Attrs: noredzone null_pointer_is_valid
 declare dso_local i64 @_copy_from_user(ptr noundef, ptr noundef, i64 noundef) local_unnamed_addr #1
 
-; Function Attrs: argmemonly mustprogress nofree noredzone nounwind null_pointer_is_valid readonly willreturn
-declare dso_local i64 @strnlen(ptr nocapture noundef, i64 noundef) local_unnamed_addr #11
-
 ; Function Attrs: noredzone null_pointer_is_valid
 declare dso_local i64 @__msecs_to_jiffies(i32 noundef) local_unnamed_addr #1
 

This appears related to the fact that __fortify_panic() is __noreturn (which introduces the unreachable instruction implicitly), as I can kill this warning by replacing strlen() with __builtin_strlen() in vhci_coredump_hdr() and it remains present by explicitly calling __fortify_panic(). It also appears related to this configuration having CONFIG_INIT_STACK_NONE, as the warning is not present with CONFIG_INIT_STACK_ALL_ZERO (the default); char buf[80] is technically uninitialized but there are other functions in drivers/bluetooth that have the exact same form but no issues. I will see if I can tease out a reproducer on top of defconfig to see if there are any other obvious factors here.

[nathanchance]

This is enough to reproduce on top of x86_64_defconfig:

CONFIG_BT=y
CONFIG_BT_HCIVHCI=m
CONFIG_INIT_STACK_ALL_ZERO=n
CONFIG_INIT_STACK_NONE=y
CONFIG_FORTIFY_SOURCE=y

Initializing buf with = “” is also sufficient to eliminate this.

I plan to try and see where in this pipeline this function gets turned into just unreachable but it is worth asking early: @kees, have you ever seen anything like this with fortify?

Another reason this might not be seen in other places within drivers/bluetooth is because these strings are all compile time constants, whereas other strings have format specifiers and additional arguments in their snprintf() calls.

[kees]

This is extremely strange. This reminds me very slightly of this issue, but that was with GCC:
https://lore.kernel.org/linux-hardening/202410171059.C2C395030@keescook/

And while it doesn't get at the Clang bug, why is snprintf involved at all here, though? String constants would be half the copying:

diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 7651321d351c..9ac22e4a070b 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -289,18 +289,18 @@ static void vhci_coredump(struct hci_dev *hdev)
 
 static void vhci_coredump_hdr(struct hci_dev *hdev, struct sk_buff *skb)
 {
-       char buf[80];
+       const char *buf;
 
-       snprintf(buf, sizeof(buf), "Controller Name: vhci_ctrl\n");
+       buf = "Controller Name: vhci_ctrl\n";
        skb_put_data(skb, buf, strlen(buf));
 
-       snprintf(buf, sizeof(buf), "Firmware Version: vhci_fw\n");
+       buf = "Firmware Version: vhci_fw\n";
        skb_put_data(skb, buf, strlen(buf));
 
-       snprintf(buf, sizeof(buf), "Driver: vhci_drv\n");
+       buf = "Driver: vhci_drv\n";
        skb_put_data(skb, buf, strlen(buf));
 
-       snprintf(buf, sizeof(buf), "Vendor: vhci\n");
+       buf = "Vendor: vhci\n";
        skb_put_data(skb, buf, strlen(buf));
 }

[kees]

I bisected the original introduction of this warning to llvm/llvm-project@1f88d80, which looks like a reasonable commit to blame.

Hm, that's an old commit. "As branch on undef is immediate undefined behavior, there is no need to mark one of the edges as feasible." And you narrowed this down to the fortified strlen() usage, which makes me think about __compiletime_strlen(), which goes poking around in strings in attempt to figure out whether it can use __builtin_strlen().

#define __compiletime_strlen(p)                                 \
({                                                              \
        char *__p = (char *)(p);                                \
        size_t __ret = SIZE_MAX;                                \
        const size_t __p_size = __member_size(p);               \  <--- this will be 80
        if (__p_size != SIZE_MAX &&                             \
            __builtin_constant_p(*__p)) {                       \ <--- this _should_ be false: it's a stack variable
                size_t __p_len = __p_size - 1;                  \
                if (__builtin_constant_p(__p[__p_len]) &&       \ <--- but if the above is true, then this will be checking an undefined
                    __p[__p_len] == '\0')                       \
                        __ret = __builtin_strlen(__p);          \
        }                                                       \
        __ret;                                                  \
})

And for some reason, Clang thinks the stack variable is constant:

https://godbolt.org/z/M9McvMxEz

__p_size: 80
__builtin_constant_p(*__p): 1
__builtin_constant_p(__p[__p_len]): 1

GCC thinks it's not.

Oh, wild. Clang is seeing through the snprintf()!

    snprintf(stack, sizeof(stack), "%s", "Ohai\n"); <- still const
    snprintf(stack, sizeof(stack), "%s", argc > 10 ? argv[0] : "Ohai\n"); <- no longer const!

That doesn't seem right -- it considers the entire string as non-const after a variable-argument snprintf(), but the trailing bytes are undef... though perhaps they are "const" undef. :| And then the __builtin_constant_p(__p[__p_len]) trips the undef logic in Clang? Ugh.

[kees]

Coccinelle script:

@const_snprintf@
expression SIZE;
identifier P;
expression STR;
@@

        char P[SIZE];
        ...
*       snprintf(P, sizeof(P), STR)
        ...
*       strlen(P)

finds these:

btintel_dmp_hdr
btmtk_coredump_hdr
btrtl_dmp_hdr
can327_handle_prompt
qca_dmp_hdr
zcore_hsa_read

which is probably not all of them -- plenty of stuff uses fortified strlen() internally.

[kees]

I am genuinely not sure where to start to fix this. Clang's behavior is weird in the sense that a stack variable is considered builtin_const after it is written with a constant string value, even though the trailing bytes are left undefined.

[kees]

This fixes it for me, but it removes some fortify smarts about const char * variables, which was kind of the point of the fancy macro:

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index e4ce1cae03bf..2d061723f0de 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -60,20 +60,9 @@ void __read_overflow2_field(size_t avail, size_t wanted) __compiletime_warning("
 void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
 void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning("detected write beyond size of field (1st parameter); maybe use struct_group()?");
 
-#define __compiletime_strlen(p)                                        \
-({                                                             \
-       char *__p = (char *)(p);                                \
-       size_t __ret = SIZE_MAX;                                \
-       const size_t __p_size = __member_size(p);               \
-       if (__p_size != SIZE_MAX &&                             \
-           __builtin_constant_p(*__p)) {                       \
-               size_t __p_len = __p_size - 1;                  \
-               if (__builtin_constant_p(__p[__p_len]) &&       \
-                   __p[__p_len] == '\0')                       \
-                       __ret = __builtin_strlen(__p);          \
-       }                                                       \
-       __ret;                                                  \
-})
+#define __compiletime_strlen(p)                                                \
+       __builtin_choose_expr(__is_constexpr(__builtin_strlen(p)),      \
+               __builtin_strlen(p), SIZE_MAX)
 
 #if defined(__SANITIZE_ADDRESS__)
 

[kees]

And FWIW, I am totally unable to construct a test that captures the "unreachable" condition. :|

[nathanchance]

Ugh, what a mess... I was able to pull out the kernel code into Godbolt for a standalone reproducer at least: https://godbolt.org/z/qj5G6e6dc

[kees]

Okay, I think this is the core of the problem:
https://godbolt.org/z/4fazEe79a

// -O2 -Wall
extern void knowable(void);
extern void unknowable(void);

void test(void)
{
    char buf[8];

    // The end of "buf" is undefined. That shouldn't make
    // __builtin_constant_p() undefined nor true, though.
    // It should just be false.
    if (__builtin_constant_p(buf[sizeof(buf)-1])) {
        knowable();
    } else {
    	unknowable();
    }
}

GCC compiles this to an unknownable() tail call. Clang compiles it to a knowable() tail call.

[kees]

Opened llvm/llvm-project#130649

[kees]

The fix I wrote in the LLVM bug appears to fix the logic and matches GCC too. I'll get some tests written over there and submit it...

[kees]

llvm/llvm-project#130713