Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\))
Subject: Re: [PATCH] bcache: Silence memcpy() run-time false positive warnings
From:   Coly Li <colyli@suse.de>
In-Reply-To: <20230106060229.never.047-kees@kernel.org>
Date:   Fri, 6 Jan 2023 21:39:16 +0800
Cc:     Thorsten Leemhuis <linux@leemhuis.info>,
        Kent Overstreet <kent.overstreet@gmail.com>,
        linux-bcache@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <CBD781CA-342F-482A-A5B0-C4EE2D7FE11B@suse.de>
References: <20230106060229.never.047-kees@kernel.org>
To:     Kees Cook <keescook@chromium.org>
Precedence: bulk


> 2023=E5=B9=B41=E6=9C=886=E6=97=A5 14:02=EF=BC=8CKees Cook =
<keescook@chromium.org> =E5=86=99=E9=81=93=EF=BC=9A
>=20
> 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:
>=20

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

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

I don=E2=80=99t 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 =E2=80=9Cstruct foo *bar[0]=E2=80=9D at the end of a data =
structure.


> Reported-by: Thorsten Leemhuis <linux@leemhuis.info>
> Link: =
https://lore.kernel.org/all/19200730-a3ba-6f4f-bb81-71339bdbbf73@leemhuis.=
info/
> Cc: Coly Li <colyli@suse.de>
> Cc: Kent Overstreet <kent.overstreet@gmail.com>
> Cc: linux-bcache@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

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

Acked-by: Coly Li <colyli@suse.de>


Coly Li


> ---
> drivers/md/bcache/bcache_ondisk.h | 3 ++-
> drivers/md/bcache/journal.c       | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>=20
> 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);
> }
>=20
> -#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 */)
>=20
> 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 =3D 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 =3D 1;
> --=20
> 2.34.1
>=20