| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 16 Oct 2023 08:41:58 -0400
From: Brian Foster <bfoster@...hat.com>
To: Kees Cook <keescook@...omium.org>
Cc: Kent Overstreet <kent.overstreet@...ux.dev>,
linux-bcachefs@...r.kernel.org, kernel test robot <lkp@...el.com>,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] bcachefs: Refactor bkey_i to use a flexible array
On Fri, Oct 13, 2023 at 04:44:21PM -0700, Kees Cook wrote:
> On Fri, Oct 13, 2023 at 07:26:11AM -0400, Brian Foster wrote:
> > On Tue, Oct 10, 2023 at 04:56:12PM -0700, Kees Cook wrote:
> > > The memcpy() in bch2_bkey_append_ptr() is operating on an embedded
> > > fake flexible array. Instead, make it explicit, and convert the memcpy
> > > to target the flexible array instead. Fixes the W=1 warning seen for
> > > -Wstringop-overflow:
> > >
> > > In file included from include/linux/string.h:254,
> > > from include/linux/bitmap.h:11,
> > > from include/linux/cpumask.h:12,
> > > from include/linux/smp.h:13,
> > > from include/linux/lockdep.h:14,
> > > from include/linux/radix-tree.h:14,
> > > from include/linux/backing-dev-defs.h:6,
> > > from fs/bcachefs/bcachefs.h:182:
> > > fs/bcachefs/extents.c: In function 'bch2_bkey_append_ptr':
> > > include/linux/fortify-string.h:57:33: warning: writing 8 bytes into a region of size 0 [-Wstringop-overflow=]
> > > 57 | #define __underlying_memcpy __builtin_memcpy
> > > | ^
> > > include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy'
> > > 648 | __underlying_##op(p, q, __fortify_size); \
> > > | ^~~~~~~~~~~~~
> > > include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk'
> > > 693 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
> > > | ^~~~~~~~~~~~~~~~~~~~
> > > fs/bcachefs/extents.c:235:17: note: in expansion of macro 'memcpy'
> > > 235 | memcpy((void *) &k->v + bkey_val_bytes(&k->k),
> > > | ^~~~~~
> > > fs/bcachefs/bcachefs_format.h:287:33: note: destination object 'v' of size 0
> > > 287 | struct bch_val v;
> > > | ^
> > >
> > > Cc: Kent Overstreet <kent.overstreet@...ux.dev>
> > > Cc: Brian Foster <bfoster@...hat.com>
> > > Cc: linux-bcachefs@...r.kernel.org
> > > Reported-by: kernel test robot <lkp@...el.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202309192314.VBsjiIm5-lkp@intel.com/
> > > Signed-off-by: Kees Cook <keescook@...omium.org>
> > > ---
> > > fs/bcachefs/bcachefs_format.h | 5 ++++-
> > > fs/bcachefs/extents.h | 2 +-
> > > 2 files changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/bcachefs/bcachefs_format.h b/fs/bcachefs/bcachefs_format.h
> > > index f0d130440baa..f5e8cb43697b 100644
> > > --- a/fs/bcachefs/bcachefs_format.h
> > > +++ b/fs/bcachefs/bcachefs_format.h
> > > @@ -300,7 +300,10 @@ struct bkey_i {
> > > __u64 _data[0];
> > >
> > > struct bkey k;
> > > - struct bch_val v;
> > > + union {
> > > + struct bch_val v;
> > > + DECLARE_FLEX_ARRAY(__u8, bytes);
> > > + };
> > > };
> >
> > Hi Kees,
> >
> > I'm curious if this is something that could be buried in bch_val given
> > it's already kind of a fake structure..? If not, my only nitty comment
>
> I was thinking it would be best to keep the flexible array has "high" in
> the struct as possible, as in the future more refactoring will be needed
> to avoid having flex arrays overlap with other members in composite
> structures. So instead of pushing into bch_val, I left it at the highest
> level possible, bch_i, as that's the struct being used by the memcpy().
>
> Eventually proper unions will be needed instead of overlapping bch_i
> with other types, as in:
>
> struct btree_root {
> struct btree *b;
>
> /* On disk root - see async splits: */
> __BKEY_PADDED(key, BKEY_BTREE_PTR_VAL_U64s_MAX);
> u8 level;
> u8 alive;
> s8 error;
> };
>
> But that's all for the future. Right now I wanted to deal with the more
> pressing matter of a 0-sized array not being zero sized. :)
>
Ok, but I'm not really following how one approach vs. the other relates
to this particular example of an embedded bkey_i. I'm probably just not
familiar enough with the current issues with 0-sized arrays and the
anticipated path forward. Can you elaborate for somebody who is more
focused on trying to manage the design/complexity of these various key
data structures? For example, what's the practical difference here (for
future work) if the flex array lives in bch_val vs. bkey_i?
Note that I don't necessarily have a strong opinion on this atm, but if
there's a "for future reasons" aspect to this approach I'd like to at
least understand it a little better. ;)
> > is that memcpy(k->bytes[], ...) makes it kind of read like we're copying
> > in opaque key data rather than value data, so perhaps a slightly more
> > descriptive field name would be helpful. But regardless I'd wait until
> > Kent has a chance to comment before changing anything..
>
> How about "v_bytes" instead of "bytes"? Or if it really is preferred,
> I can move the flex array into bch_val -- it just seems like the wrong
> layer...
>
Yeah.. v_bytes, value_bytes, etc. etc. Anything that avoids misleading
code when using the field is good with me. Thanks.
Brian
> -Kees
>
> >
> > Brian
> >
> > >
> > > #define KEY(_inode, _offset, _size) \
> > > diff --git a/fs/bcachefs/extents.h b/fs/bcachefs/extents.h
> > > index 7ee8d031bb6c..6248e17bbac5 100644
> > > --- a/fs/bcachefs/extents.h
> > > +++ b/fs/bcachefs/extents.h
> > > @@ -642,7 +642,7 @@ static inline void bch2_bkey_append_ptr(struct bkey_i *k, struct bch_extent_ptr
> > >
> > > ptr.type = 1 << BCH_EXTENT_ENTRY_ptr;
> > >
> > > - memcpy((void *) &k->v + bkey_val_bytes(&k->k),
> > > + memcpy(&k->bytes[bkey_val_bytes(&k->k)],
> > > &ptr,
> > > sizeof(ptr));
> > > k->k.u64s++;
> > > --
> > > 2.34.1
> > >
> >
>
> --
> Kees Cook
>
Powered by blists - more mailing lists