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;
Date:   Fri, 6 Jan 2023 11:53:23 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Coly Li <colyli@suse.de>
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
Subject: Re: [PATCH] bcache: Silence memcpy() run-time false positive warnings
Message-ID: <202301061125.2041FE82D@keescook>
References: <20230106060229.never.047-kees@kernel.org>
 <CBD781CA-342F-482A-A5B0-C4EE2D7FE11B@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CBD781CA-342F-482A-A5B0-C4EE2D7FE11B@suse.de>
Precedence: bulk

On Fri, Jan 06, 2023 at 09:39:16PM +0800, Coly Li wrote:
> 
> 
> > 2023年1月6日 14:02，Kees Cook <keescook@chromium.org> 写道：
> > 
> > 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@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>

Thanks!

-Kees

-- 
Kees Cook