-Wattribute-warning in drivers/net/ethernet/huawei/hinic/hinic_devlink.c

[nathanchance]

With both ARCH=arm64 allmodconfig and ARCH=x86_64 allmodconfig on linux-next, I see:

$ make -skj"$(nproc)" LLVM=1 mrproper allmodconfig drivers/net/ethernet/huawei/hinic/hinic_devlink.o
In file included from drivers/net/ethernet/huawei/hinic/hinic_devlink.c:15:
In file included from ./include/linux/netlink.h:7:
In file included from ./include/linux/skbuff.h:15:
In file included from ./include/linux/time.h:60:
In file included from ./include/linux/time32.h:13:
In file included from ./include/linux/timex.h:65:
In file included from ./arch/x86/include/asm/timex.h:5:
In file included from ./arch/x86/include/asm/processor.h:22:
In file included from ./arch/x86/include/asm/msr.h:11:
In file included from ./arch/x86/include/asm/cpumask.h:5:
In file included from ./include/linux/cpumask.h:12:
In file included from ./include/linux/bitmap.h:11:
In file included from ./include/linux/string.h:253:
./include/linux/fortify-string.h:328:4: error: call to __write_overflow_field declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
                        __write_overflow_field(p_size_field, size);
                        ^
1 error generated.

This comes from the new fortify checks. Specifically, it appears to come from the memcpy() in check_image_valid():

diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
index 60ae8bfc5f69..11a52cc375a2 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
@@ -43,9 +43,11 @@ static bool check_image_valid(struct hinic_devlink_priv *priv, const u8 *buf,

        for (i = 0; i < fw_image->fw_info.fw_section_cnt; i++) {
                len += fw_image->fw_section_info[i].fw_section_len;
+               /*
                memcpy(&host_image->image_section_info[i],
                       &fw_image->fw_section_info[i],
                       sizeof(struct fw_section_info_st));
+               */
        }

        if (len != fw_image->fw_len ||

static bool check_image_valid(struct hinic_devlink_priv *priv, const u8 *buf,
                  u32 image_size, struct host_image_st *host_image)
{
    struct fw_image_st *fw_image = NULL;
    u32 len = 0;
    u32 i;

    fw_image = (struct fw_image_st *)buf;

    if (fw_image->fw_magic != HINIC_MAGIC_NUM) {
        dev_err(&priv->hwdev->hwif->pdev->dev, "Wrong fw_magic read from file, fw_magic: 0x%x\n",
            fw_image->fw_magic);
        return false;
    }

    if (fw_image->fw_info.fw_section_cnt > MAX_FW_TYPE_NUM) {
        dev_err(&priv->hwdev->hwif->pdev->dev, "Wrong fw_type_num read from file, fw_type_num: 0x%x\n",
            fw_image->fw_info.fw_section_cnt);
        return false;
    }

    for (i = 0; i < fw_image->fw_info.fw_section_cnt; i++) {
        len += fw_image->fw_section_info[i].fw_section_len;
        memcpy(&host_image->image_section_info[i],
               &fw_image->fw_section_info[i],
               sizeof(struct fw_section_info_st));
    }
...

From drivers/net/ethernet/huawei/hinic/hinic_devlink.h:

...
#define MAX_FW_TYPE_NUM 30
...
struct fw_section_info_st {
	u32 fw_section_len;
	u32 fw_section_offset;
	u32 fw_section_version;
	u32 fw_section_type;
	u32 fw_section_crc;
};

struct fw_image_st {
	u32 fw_version;
	u32 fw_len;
	u32 fw_magic;
	struct {
		u32 fw_section_cnt:16;
		u32 resd:16;
	} fw_info;
	struct fw_section_info_st fw_section_info[MAX_FW_TYPE_NUM];
	u32 device_id;
	u32 res[101];
	void *bin_data;
};

struct host_image_st {
	struct fw_section_info_st image_section_info[MAX_FW_TYPE_NUM];
	struct {
		u32 up_total_len;
		u32 fw_version;
	} image_info;
	u32 section_type_num;
	u32 device_id;
};

I am struggling to see what clang finds problematic here?

cc @kees

[kees]

Uuuhhh. That's weird. sizeof(host_image->image_section_info[i]) == sizeof(struct fw_section_info_st). And it doesn't happen with clang-13. Digging...

[kees]

https://godbolt.org/z/hvvvczjh9

I am disappointed in __bos()'s behavior generally with this array, and am confused about how to elicit the memcpy() warning. I was expecting the bos to return 600 or something...

More tomorrow...

[kees]

This continues to be a problem, though only with Clang 14+.

[nathanchance]

I am guessing this is only visible with Clang 14+ because that is when the warning attribute was added, not because of a regression. I could try cherry-picking the warning attribute patch back to Clang 13 to see.

[kees]

AIUI, In Clang 13 and earlier, it should manifest as a link-time failure?

[nathanchance]

Hmmm, I don't think so, as __write_overflow_field() is defined in lib/string_helpers.c, unless you mean a different link-time failure?

[kees]

Well, well, well! This only manifests with -fsanitize=array-bounds.

[nathanchance]

Still an issue on 5.19-rc2. It should be possible to work around this with unsafe_memcpy():

diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
index 60ae8bfc5f69..4642d69b84ac 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
@@ -43,9 +43,10 @@ static bool check_image_valid(struct hinic_devlink_priv *priv, const u8 *buf,

        for (i = 0; i < fw_image->fw_info.fw_section_cnt; i++) {
                len += fw_image->fw_section_info[i].fw_section_len;
-               memcpy(&host_image->image_section_info[i],
-                      &fw_image->fw_section_info[i],
-                      sizeof(struct fw_section_info_st));
+               unsafe_memcpy(&host_image->image_section_info[i],
+                             &fw_image->fw_section_info[i],
+                             sizeof(struct fw_section_info_st),
+                             "CONFIG_UBSAN_ARRAY_BOUNDS can confuse the fortify checks");
        }

        if (len != fw_image->fw_len ||

[kees]

    135f:       bf 10 00 00 00          mov    $0x10,%edi
    1364:       be 14 00 00 00          mov    $0x14,%esi
    1369:       e8 00 00 00 00          call   136e <hinic_devlink_flash_update+0x102e>
                        136a: R_X86_64_PLT32    __write_overflow_field-0x4

so:

p_size_field == 0x10
size == 0x14

size is correct. p_size_field is impossible. Nothing here is 0x10 in size! struct fw_section_info_st is 0x14.

And... omg, why did it take me so long to realize this could just be a direct assignment. Patch sent:
https://lore.kernel.org/linux-hardening/20220616052312.292861-1-keescook@chromium.org/T/#u

[nickdesaulniers]

I wonder if perhaps some members of the struct are determined to be unused statically. Just a guess.

[nathanchance]

Patch accepted: https://git.kernel.org/netdev/net-next/c/2c0ab32b73cfe39a609192f338464e948fc39117

[nickdesaulniers]

In this case KCFLAGS=-Rpass=inline can give us some hints:

$ make LLVM=1 -j72 drivers/net/ethernet/huawei/hinic/hinic_devlink.o KCFLAGS=-Rpass=inline
...
drivers/net/ethernet/huawei/hinic/hinic_devlink.c:46:3: remark: '_Z18fortify_memcpy_chkmmmmmPKc' inlined into 'check_image_valid': always inline attribute at callsite check_image_valid:23:3; [-Rpass=inline]
                memcpy(&host_image->image_section_info[i],
                ^
...

It's not great, but until we have #1571 fixed, this will probably be the fastest way to identify such issues with clang.

[nickdesaulniers]

Also strange:

$ make LLVM=1 -j72 drivers/net/ethernet/huawei/hinic/hinic_devlink.ll

The only call to @memcpy in the source is:

%28 = call ptr @memcpy(ptr %arrayidx22125137.i.i, ptr %arrayidx118124139.i.i, i64 20)

(20 == 0x14)
In the disassembly, I also only see:

     566: ba 14 00 00 00                movl    $20, %edx   # i.e. n == 20 == 0x14
     56b: 48 8b 6c 24 28                movq    40(%rsp), %rbp
     570: 48 89 ef                      movq    %rbp, %rdi
     573: 4c 89 ee                      movq    %r13, %rsi
     576: e8 00 00 00 00                callq   0x57b <hinic_devlink_flash_update+0x23b>
                0000000000000577:  R_X86_64_PLT32       memcpy-0x4

Well, well, well! This only manifests with -fsanitize=array-bounds.

Ok, but defconfig + UBSAN_BOUNDS does not.

$ make LLVM=1 -j72 defconfig
$ ./scripts/config -e UBSAN -e UBSAN_BOUNDS -e NET_VENDOR_HUAWEI -e HINIC
$ make LLVM=1 -j72 olddefconfig drivers/net/ethernet/huawei/hinic/hinic_devlink.o

so it seems like CONFIG_UBSAN_BOUNDS plus some other currently-unknown flag from allmodconfig are tickling this.

[nathanchance]

Ok, but defconfig + UBSAN_BOUNDS does not.

$ make LLVM=1 -j72 defconfig
$ ./scripts/config -e UBSAN -e UBSAN_BOUNDS -e NET_VENDOR_HUAWEI -e HINIC
$ make LLVM=1 -j72 olddefconfig drivers/net/ethernet/huawei/hinic/hinic_devlink.o

so it seems like CONFIG_UBSAN_BOUNDS plus some other currently-unknown flag from allmodconfig are tickling this.

defconfig does not enable CONFIG_FORTIFY_SOURCE.

$ git show -s --format='%h ("%s")'
de5c208d533a ("Merge tag 'linux-kselftest-fixes-5.19-rc4' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest")

$ make -skj"$(nproc)" LLVM=1 mrproper defconfig

$ scripts/config -e UBSAN -e UBSAN_BOUNDS -e NET_VENDOR_HUAWEI -e HINIC -e FORTIFY_SOURCE

$ make -skj"$(nproc)" LLVM=1 olddefconfig drivers/net/ethernet/huawei/hinic/hinic_devlink.o
In file included from drivers/net/ethernet/huawei/hinic/hinic_devlink.c:15:
In file included from ./include/linux/netlink.h:7:
In file included from ./include/linux/skbuff.h:15:
In file included from ./include/linux/time.h:60:
In file included from ./include/linux/time32.h:13:
In file included from ./include/linux/timex.h:67:
In file included from ./arch/x86/include/asm/timex.h:5:
In file included from ./arch/x86/include/asm/processor.h:22:
In file included from ./arch/x86/include/asm/msr.h:11:
In file included from ./arch/x86/include/asm/cpumask.h:5:
In file included from ./include/linux/cpumask.h:12:
In file included from ./include/linux/bitmap.h:11:
In file included from ./include/linux/string.h:253:
./include/linux/fortify-string.h:344:4: error: call to __write_overflow_field declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
                        __write_overflow_field(p_size_field, size);
                        ^
1 error generated.
...

[nathanchance]

This warning is fixed in mainline: https://git.kernel.org/linus/1e70212e031528918066a631c9fdccda93a1ffaa

I'll request a stable backport on Monday to give it a little soak time in mainline.

[nickdesaulniers]

So in this case the compile thinks that __builtin_object_size(&host_image->image_section_info[i], 1) < sizeof(struct fw_section_info_st). @kees do you remember what the rules are for BOS(1)? (I don't).

[kees]

So in this case the compile thinks that __builtin_object_size(&host_image->image_section_info[i], 1) < sizeof(struct fw_section_info_st). @kees do you remember what the rules are for BOS(1)? (I don't).

Right; this seems impossible. The struct is 5 u32s (0x14), but it resolves to 0x10. There's no padding, there are no hole, etc, etc.

[kees]

Here are two other cases of the mysterious shrinking __bos:

• https://lore.kernel.org/lkml/20220920192202.190793-2-keescook@chromium.org/
• -Wattribute-warning in drivers/crypto/hisilicon/sgl.c #1780