On Mon, Oct 24, 2022 at 09:59:08AM -0700, Kees Cook wrote:
> On Wed, Oct 19, 2022 at 05:04:11PM -0700, Darrick J. Wong wrote:
> > [...]
> > -/*
> > - * Copy an BUI format buffer from the given buf, and into the destination
> > - * BUI format structure. The BUI/BUD items were designed not to need any
> > - * special alignment handling.
> > - */
> > -static int
> > -xfs_bui_copy_format(
> > - struct xfs_log_iovec *buf,
> > - struct xfs_bui_log_format *dst_bui_fmt)
> > -{
> > - struct xfs_bui_log_format *src_bui_fmt;
> > - uint len;
> > -
> > - src_bui_fmt = buf->i_addr;
> > - len = xfs_bui_log_format_sizeof(src_bui_fmt->bui_nextents);
> > -
> > - if (buf->i_len == len) {
> > - memcpy(dst_bui_fmt, src_bui_fmt, len);
> > - return 0;
> > - }
> > - XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> > - return -EFSCORRUPTED;
> > -}
>
> This is the place where flex_cpy() could be used:
>
> flex_cpy(dst_bui_fmt, src_bui_fmt);
<nod>
> > [...]
> > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > index 51f66e982484..5367e404aa0f 100644
> > --- a/fs/xfs/xfs_bmap_item.c
> > +++ b/fs/xfs/xfs_bmap_item.c
> > @@ -590,7 +590,7 @@ xfs_bui_item_relog(
> > set_bit(XFS_LI_DIRTY, &budp->bud_item.li_flags);
> >
> > buip = xfs_bui_init(tp->t_mountp);
> > - memcpy(buip->bui_format.bui_extents, extp, count * sizeof(*extp));
> > + memcpy_array(buip->bui_format.bui_extents, extp, count, sizeof(*extp));
> > atomic_set(&buip->bui_next_extent, count);
> > xfs_trans_add_item(tp, &buip->bui_item);
> > set_bit(XFS_LI_DIRTY, &buip->bui_item.li_flags);
>
> Looking more closely, I don't understand why this is treated as a flex
> array when it's actually fixed size:
>
> xfs_bui_init():
> buip = kmem_cache_zalloc(xfs_bui_cache, GFP_KERNEL | __GFP_NOFAIL);
> ...
> buip->bui_format.bui_nextents = XFS_BUI_MAX_FAST_EXTENTS;
>
> fs/xfs/xfs_bmap_item.h:#define XFS_BUI_MAX_FAST_EXTENTS 1
Yeah, after a few more iterations of this patchset I realized that
*most* of the _relog functions are fine, it's only the one for EFI items
that trips over the not-flex array[1] definition. I decided that the
proper fix for that was simply to fix the field definition to follow the
modern form for flex arrays.
> > [...]
> > +/*
> > + * Copy an array from @src into the @dst buffer, allowing for @dst to be a
> > + * structure with a VLAs at the end. gcc11 is smart enough for
> > + * __builtin_object_size to see through void * arguments to static inline
> > + * function but not to detect VLAs, which leads to kernel warnings.
> > + */
> > +static inline int memcpy_array(void *dst, void *src, size_t nmemb, size_t size)
> > +{
> > + size_t bytes;
> > +
> > + if (unlikely(check_mul_overflow(nmemb, size, &bytes))) {
> > + ASSERT(0);
> > + return -ENOMEM;
> > + }
> > +
> > + unsafe_memcpy(dst, src, bytes, VLA size detection broken on gcc11 );
> > + return 0;
> > +}
>
> This "unsafe_memcpy" isn't needed. FORTIFY won't warn on this copy:
> the destination is a flex array member, not a flex array struct
> (i.e. __builtin_object_size() here will report "-1", rather than a
> fixed size). And while the type bounds checking for overflow is nice,
> it should also be checking the allocated size. (i.e. how large is "dst"?
> this helper only knows how large src is.)
<nod> I realized that these helpers introducing unsafe memcpy weren't
needed. Later on after chatting with dchinner a bit I came to the
conclusion that we might as well convert most of the _copy_format
functions to memcpy the structure head and flex array separately since
that function is converting an ondisk log item into its in-memory
representation, and some day we'll make those struct fields endian safe.
They aren't now, and that's one of the (many) gaping holes that need
fixing.
I sent my candidate fixes series to the list just now.
--D
>
> -Kees
>
> --
> Kees Cook
