Re: [linux-next:master] BUILD REGRESSION df964ce9ef9fea10cf131bf6bad8658fde7956f6

[Kees Cook <keescook@chromium.org>]

Hi Kent,

Andrew pointed this out to me, and it's a FORTIFY issue under a W=1 build:

On Sat, Sep 30, 2023 at 01:25:34PM +0800, kernel test robot wrote:
> tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> branch HEAD: df964ce9ef9fea10cf131bf6bad8658fde7956f6  Add linux-next specific files for 20230929
> 
> Error/Warning reports:
> 
> [...]
> https://lore.kernel.org/oe-kbuild-all/202309192314.VBsjiIm5-lkp@intel.com

   fs/bcachefs/extents.c: In function 'bch2_bkey_append_ptr':
   include/linux/fortify-string.h:57:33: warning: writing 8 bytes into a region of size 0 [-Wstringop-overflow=]
      57 | #define __underlying_memcpy     __builtin_memcpy
         |                                 ^
   include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy'
     648 |         __underlying_##op(p, q, __fortify_size);                        \
         |         ^~~~~~~~~~~~~
   include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk'
     693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
         |                          ^~~~~~~~~~~~~~~~~~~~
   fs/bcachefs/extents.c:235:17: note: in expansion of macro 'memcpy'
     235 |                 memcpy((void *) &k->v + bkey_val_bytes(&k->k),
         |                 ^~~~~~
   fs/bcachefs/bcachefs_format.h:287:33: note: destination object 'v' of size 0
     287 |                 struct bch_val  v;
         |                                 ^

The problem here is the struct bch_val is explicitly declared as a
zero-sized array, so the compiler becomes unhappy. :) Converting bch_val
to a flexible array will just kick the can down the road, since this is
going to run into -Wflex-array-member-not-at-end soon too since bch_val
overlaps with other structures:

struct bch_inode_v3 {
        struct bch_val          v;

        __le64                  bi_journal_seq;
...
};

As a container_of() target, this is fine -- leave it a zero-sized
array. The problem is using it as a destination for memcpy, etc, since
the compiler will believe it to be 0 sized. Instead, we need to impart
a type of some kind so that the compiler can actually unambiguously
reason about sizes. The memcpy() in the warning is targeting bch_val,
so I think the best fix is to correctly handle the different types.

So just to have everything in front of me, here's a summary of what I'm
seeing in the code:

struct bkey {
        /* Size of combined key and value, in u64s */
        __u8            u64s;
...
};

/* Empty placeholder struct, for container_of() */
struct bch_val {
        __u64           __nothing[0];
};

struct bkey_i {
        __u64                   _data[0];

        struct bkey     k;
        struct bch_val  v;
};

static inline void bch2_bkey_append_ptr(struct bkey_i *k, struct bch_extent_ptr ptr)
{
        EBUG_ON(bch2_bkey_has_device(bkey_i_to_s(k), ptr.dev));

        switch (k->k.type) {
        case KEY_TYPE_btree_ptr:
        case KEY_TYPE_btree_ptr_v2:
        case KEY_TYPE_extent:
                EBUG_ON(bkey_val_u64s(&k->k) >= BKEY_EXTENT_VAL_U64s_MAX);

                ptr.type = 1 << BCH_EXTENT_ENTRY_ptr;

                memcpy((void *) &k->v + bkey_val_bytes(&k->k),
                       &ptr,
                       sizeof(ptr));
                k->k.u64s++;
                break;
        default:
                BUG();
        }
}

So this is appending u64s into the region that start with bkey_i. Could
this gain a u64 flexible array?

struct bkey_i {
        __u64                   _data[0];

        struct bkey     k;
        struct bch_val  v;
	__u64		ptrs[];
};

Then the memcpy() could be just a direct assignment:

		k->ptrs[bkey_val_u64s(&k->k)] = (u64)ptr;
                k->k.u64s++;

Alternatively, perhaps struct bkey could be the one to grow this flexible
array, and then it could eventually be tracked with __counted_by (but
not today since it expects to count the array element count, not a whole
struct size):

struct bkey {
        /* Size of combined key and value, in u64s */
        __u8            u64s;
...
	__u64		data[] __counted_by(.u64s - BKEY_U64s);
};

And bch_val could be dropped...

Then the memcpy becomes:

                k->k.u64s++;
                k->k.data[bkey_val_u64s(&k->k)] = (u64)ptr;

It just seems like there is a lot of work happening that could really
just type casting or unions...

What do you think?

-- 
Kees Cook