bcache: Silence memcpy() run-time false positive warnings

struct bkey has internal padding in a union, but it isn't always named the same (e.g. key ## _pad, key_p, etc). This makes it extremely hard for the compiler to reason about the available size of copies done against such keys. Use unsafe_memcpy() for now, to silence the many run-time false positive warnings:

memcpy: detected field-spanning write (size 264) of single field "&i->j" at drivers/md/bcache/journal.c:152 (size 240)
memcpy: detected field-spanning write (size 24) of single field "&b->key" at drivers/md/bcache/btree.c:939 (size 16)
emcpy: detected field-spanning write (size 24) of single field "&temp.key" at drivers/md/bcache/extents.c:428 (size 16)

---
 drivers/md/bcache/bcache_ondisk.h | 3 ++-
 drivers/md/bcache/journal.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

index d086a0ce4bc2..6620a7f8fffc 100644
@@ -106,7 +106,8 @@ static inline unsigned long bkey_bytes(const struct bkey *k)
 	return bkey_u64s(k) * sizeof(__u64);
 }
 
-#define bkey_copy(_dest, _src)	memcpy(_dest, _src, bkey_bytes(_src))
+#define bkey_copy(_dest, _src)	unsafe_memcpy(_dest, _src, bkey_bytes(_src), \
+					/* bkey is always padded */)
 
 static inline void bkey_copy_key(struct bkey *dest, const struct bkey *src)
 {

index e5da469a4235..c182c21de2e8 100644
@@ -149,7 +149,8 @@ reread:		left = ca->sb.bucket_size - offset;
 				    bytes, GFP_KERNEL);
 			if (!i)
 				return -ENOMEM;
-			memcpy(&i->j, j, bytes);
+			unsafe_memcpy(&i->j, j, bytes,
+				/* "bytes" was calculated by set_bytes() above */);
 			/* Add to the location after 'where' points to */
 			list_add(&i->list, where);
 			ret = 1;

2.34.1

Thx for looking into this.

Credit where credit is due, this should be:

Ciao, Thorsten

The keys is embedded in multiple data structures as a generalized model with some helpers, bkey_bytes() is one of them.

It is not surprised for such feeling when people read the code at first glance. And thank you for improving it :-)

How does the above information can be founded? Should I use llvm and enable FORTIFY_SOURCE?

I don’t say the bkey and bkey_bytes() stuffs are elegant, but why the compiler cannot find such situation? IMHO it is quite similar to something like “struct foo *bar[0]” at the end of a data structure.

The code comments as justification is informative. Thanks for this.

Coly Li

Ah-ha, thank you! I've updated this.

Kees Cook

Yeah, if the helpers were able to operate on the padding variable (rather than the key), then the compiler would be in a better position to figure out bounds checking. Right now it sees the destination as the key and then the size as larger than the key (but equal to the union's padding variable -- but it doesn't "know" about that).

It was reported at run-time under a kernel built with CONFIG_FORTIFY_SOURCE=y (See https://bugzilla.kernel.org/show_bug.cgi?id=216785)

Nit: "bar[0]" is deprecated in favor of "bar[]" or DECLARE_FLEX_ARRAY(...) where needed, see:
https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays
and here is the patch that fixes this:
https://lore.kernel.org/all/YzIc8z+QaHvqPjLX@work/

But to answer the question, what's happening is mostly due to a (specification?) bug in GCC and Clang (where it can't tell an outer struct contains an inner struct that ends in a flexible array), so this:

struct jset {
	...
        union {
                struct bkey     start[0];
                __u64           d[0];
        };
};

looks like it has a fixed size, when in fact it doesn't.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
There is disagreement within GCC about whether or not this behavior is "by design", so for the meantime, we have to work around it in the kernel.

So the first warning comes from:

	memcpy(&i->j, j, bytes);

where the compiler thinks the object pointed at by "&i->j" is fixed size, as it doesn't "see" the trailing flexible arrays within j.

The other 2 warnings come from the same problem, but they're from struct bkey and the union created by BKEY_PADDED(). Here's everything I could see:

struct bkey {
        __u64   high;
        __u64   low;
        __u64   ptr[];
};

#define BKEY_PADDED(key)                                        \
        union { struct bkey key; __u64 key ## _pad[BKEY_PAD]; }

#define BITMASK(name, type, field, offset, size)                \
static inline __u64 name(const type *k)                         \
{ return (k->field >> offset) & ~(~0ULL << size); }             \

#define KEY_FIELD(name, field, offset, size) \
BITMASK(name, struct bkey, field, offset, size)

KEY_FIELD(KEY_PTRS, high, 60, 3)

static inline unsigned long bkey_u64s(const struct bkey *k)

{
        return (sizeof(struct bkey) / sizeof(__u64)) + KEY_PTRS(k);
}

static inline unsigned long bkey_bytes(const struct bkey *k)

{
        return bkey_u64s(k) * sizeof(__u64);
}

#define bkey_copy(_dest, _src) memcpy(_dest, _src, bkey_bytes(_src))

	BKEY_PADDED(key) temp;
	bkey_copy(&temp.key, k);

So, again, the memcpy() in bkey_copy() can't see into &temp.key to notice the trailing flexible array, and thinks it is copying "too much".

However, in both cases, the bounds checking is being performed (first by set_bytes(), and in the latter two, by bkey_bytes()), so the direct fix is to just disable the FORTIFY checking on these memcpy() uses.

Thanks!

-Kees

Kees Cook