On Fri, Jan 06, 2023 at 09:39:16PM +0800, Coly Li wrote: > > > > 2023年1月6日 14:02，Kees Cook <keescook@xxxxxxxxxxxx> 写道： > > > > 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: > > > > The keys is embedded in multiple data structures as a generalized model with some helpers, bkey_bytes() is one of them. 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). > > 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) > > memcpy: detected field-spanning write (size 24) of single field "&temp.key" at drivers/md/bcache/extents.c:428 (size 16) > > > > How does the above information can be founded? Should I use llvm and enable FORTIFY_SOURCE? 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) > 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. 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. > > Reported-by: Thorsten Leemhuis <linux@xxxxxxxxxxxxx> > > Link: https://lore.kernel.org/all/19200730-a3ba-6f4f-bb81-71339bdbbf73@xxxxxxxxxxxxx/ > > Cc: Coly Li <colyli@xxxxxxx> > > Cc: Kent Overstreet <kent.overstreet@xxxxxxxxx> > > Cc: linux-bcache@xxxxxxxxxxxxxxx > > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > > The code comments as justification is informative. Thanks for this. > > Acked-by: Coly Li <colyli@xxxxxxx> Thanks! -Kees -- Kees Cook
