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

Commit Message

Kees Cook Jan. 6, 2023, 6:02 a.m. UTC

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)

Reported-by: Thorsten Leemhuis <[e-mail]>
Link: https://lore.kernel.org/all/[e-mail]/
Cc: Coly Li <[e-mail]>
Cc: Kent Overstreet <[e-mail]>
Cc: [e-mail]
Signed-off-by: Kees Cook <[e-mail]>
---
 drivers/md/bcache/bcache_ondisk.h | 3 ++-
 drivers/md/bcache/journal.c       | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Thorsten Leemhuis Jan. 6, 2023, 10:01 a.m. UTC | #1

On 06.01.23 07:02, Kees Cook wrote:
> 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)

Thx for looking into this.

> Reported-by: Thorsten Leemhuis <[e-mail]>
> Link: https://lore.kernel.org/all/[e-mail]/

Credit where credit is due, this should be:

Reported-by: Alexandre Pereira <[e-mail]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216785

> Cc: Coly Li <[e-mail]>
> […]

Ciao, Thorsten

Coly Li Jan. 6, 2023, 1:39 p.m. UTC | #2

> 2023年1月6日 14:02，Kees Cook <[e-mail]> 写道：
> 
> 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.

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


>  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)
> 

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.


> Reported-by: Thorsten Leemhuis <[e-mail]>
> Link: https://lore.kernel.org/all/[e-mail]/
> Cc: Coly Li <[e-mail]>
> Cc: Kent Overstreet <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Kees Cook <[e-mail]>

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

Acked-by: Coly Li <[e-mail]>


Coly Li


> ---
> drivers/md/bcache/bcache_ondisk.h | 3 ++-
> drivers/md/bcache/journal.c       | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/bcache_ondisk.h b/drivers/md/bcache/bcache_ondisk.h
> index d086a0ce4bc2..6620a7f8fffc 100644
> --- a/drivers/md/bcache/bcache_ondisk.h
> +++ b/drivers/md/bcache/bcache_ondisk.h
> @@ -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)
> {
> diff --git a/drivers/md/bcache/journal.c b/drivers/md/bcache/journal.c
> index e5da469a4235..c182c21de2e8 100644
> --- a/drivers/md/bcache/journal.c
> +++ b/drivers/md/bcache/journal.c
> @@ -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
>

Kees Cook Jan. 6, 2023, 7:24 p.m. UTC | #3

On Fri, Jan 06, 2023 at 11:01:05AM +0100, Thorsten Leemhuis wrote:
> Credit where credit is due, this should be:
> 
> Reported-by: Alexandre Pereira <[e-mail]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216785

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

Kees Cook Jan. 6, 2023, 7:53 p.m. UTC | #4

On Fri, Jan 06, 2023 at 09:39:16PM +0800, Coly Li wrote:
> 
> 
> > 2023年1月6日 14:02，Kees Cook <[e-mail]> 写道：
> > 
> > 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 <[e-mail]>
> > Link: https://lore.kernel.org/all/[e-mail]/
> > Cc: Coly Li <[e-mail]>
> > Cc: Kent Overstreet <[e-mail]>
> > Cc: [e-mail]
> > Signed-off-by: Kees Cook <[e-mail]>
> 
> The code comments as justification is informative. Thanks for this.
> 
> Acked-by: Coly Li <[e-mail]>

Thanks!

-Kees

Patch

diff --git a/drivers/md/bcache/bcache_ondisk.h b/drivers/md/bcache/bcache_ondisk.h
index d086a0ce4bc2..6620a7f8fffc 100644
--- a/drivers/md/bcache/bcache_ondisk.h
+++ b/drivers/md/bcache/bcache_ondisk.h
@@ -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)
 {
diff --git a/drivers/md/bcache/journal.c b/drivers/md/bcache/journal.c
index e5da469a4235..c182c21de2e8 100644
--- a/drivers/md/bcache/journal.c
+++ b/drivers/md/bcache/journal.c
@@ -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;