Message-ID: <20221024223235.GA3600936@dread.disaster.area>
Date:   Tue, 25 Oct 2022 09:32:35 +1100
From:   Dave Chinner <david@...morbit.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     "Darrick J. Wong" <djwong@...nel.org>,
        xfs <linux-xfs@...r.kernel.org>, Zorro Lang <zlang@...hat.com>,
        linux-hardening@...r.kernel.org
Subject: Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item
 memcpy

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

How does flex_cpy() know how much memory was allocated for
dst_bui_fmt? Doesn't knowing this imply that we have to set the
count field in dst_bui_fmt appropriately before flex_cpy() is
called?

If this is the case, this flex_cpy() thing just looks like it's
moving the problem around, not actually solving any problem in this
code. If anything, it is worse, because it is coupling the size of
the copy to a structure internal initialisation value that may be
nowhere near the code that does the copy. That makes the code much
harder to validate by reading it.

Indeed, by the time we get to the memcpy() above, we've validated
length two ways, we allocated dst_bui_fmt to fit that length, and we
know that the src_bui_fmt length is, well, length, because that's
what the higher level container structure told us it's length was.
And with memcpy() being passed that length, it is *obviously
correct* to the reader.

Hence I don't see that this flex array copying stuff will make it
harder to make mistakes, but ISTM that it'll make them harder to spot
during review and audit...

> > [...]
> > 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

We have a separation between on-disk format structure parsing
template that implements the BUI/BUD code (i.e. same implementation
as EFI, RUI, and CUI intents) and the runtime code that is currently
only using a single extent in the flex array.

The use of template based implementations means modifications are
simple (if repetitive) and we don't have to think about specific
intent implementations differently when reasoning about extent-based
intent defering and recovery. 

Further, the high level code that creates BUIs could change to use
multiple extents at any time. We don't want to have to rewrite the
entire log item formatting and parsing code every time we change the
number of extents we currently track in a given intent....

> > [...]
> > +/*
> > + * 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.)

The caller knows how large dst is - it's based on the verified size
of the structure it is going to copy. Compile time checking the
copy doesn't oblivate the need to perform runtime checking needed to
to set up the copy correctly/safely...

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists