Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item memcpy

Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item memcpy

[Kees Cook]

On Wed, Oct 19, 2022 at 05:04:11PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of
> memcpy.  Unfortunately, it doesn't handle VLAs correctly:

Nit-pick on terminology: these are "flexible array structures" (structures
that end with a "flexible array member"); VLAs are a different (removed
from the kernel) beast.

> memcpy: detected field-spanning write (size 48) of single field "dst_bui_fmt" at fs/xfs/xfs_bmap_item.c:628 (size 16)

Step right up; XFS is next to trip[1] this check. Let's get this fixed...

> We know the memcpy going
> on here is correct because I've run all the log recovery tests with
> KASAN turned on, and it does not detect actual memory misuse.

Yup, this is a false positive.

> My first attempt to work around this problem was to cast the arguments
> [...]
> My second attempt changed the cast to a (void *), with the same results
> [...]
> My third attempt was to pass the void pointers directly into
> [...]
> My fourth attempt collapsed the _copy_format function into the callers
> [...]

The point here is to use a better API, which is fallible and has the
ability to perform the bounds checking itself. I had proposed an initial
version of this idea here[2].

[1] https://lore.kernel.org/all/?q=%22field-spanning+write%22
[2] https://lore.kernel.org/llvm/20220504014440.3697851-3-keescook@chromium.org/

> "These cases end up appearing to the compiler to be sized as if the
> flexible array had 0 elements. :( For more details see:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
> https://godbolt.org/z/vW6x8vh4P ".
> 
> I don't /quite/ think that turning off CONFIG_FORTIFY_SOURCE is the
> right solution here, but in the meantime this is causing a lot of fstest
> failures, and I really need to get back to fixing user reported data
> corruption problems instead of dealing with gcc stupidity. :(

I think XFS could be a great first candidate for using something close
to the proposed flex_cpy() API. What do you think of replacing the
memcpy() calls with something like this instead:

-	if (buf->i_len == len) {
-		memcpy(dst_bui_fmt, src_bui_fmt, len);
-		return 0;
-	}
+	if (buf->i_len == len &&
+	    flex_cpy(dst_bui_fmt, src_bui_fmt,
+		     bui_nextents, bui_extents) == 0)
		return 0;
	XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
	return -EFSCORRUPTED;

To avoid passing in the element count and element array fields, the
alias macros could be used:

struct xfs_bui_log_format {
	uint16_t		bui_type;	/* bui log item type */
	uint16_t		bui_size;	/* size of this item */
	/* # extents to free */
	DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(uint32_t, bui_nextents);
	uint64_t		bui_id;		/* bui identifier */
	/* array of extents to bmap */
	DECLARE_FLEX_ARRAY_ELEMENTS(struct xfs_map_extent, bui_extents);
};

What do you think about these options? In the meantime, unsafe_memcpy()
should be fine for v6.1.

BTW, this FORTIFY_SOURCE change was present in linux-next for the entire
prior development cycle. Are the xfstests not run on -next kernels?

-Kees

-- 
Kees Cook

Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item memcpy

[Kees Cook]

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

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

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

-Kees

-- 
Kees Cook

Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item memcpy

[Darrick J. Wong]

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

Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item memcpy

[Kees Cook]

On Mon, Oct 24, 2022 at 02:38:14PM -0700, Darrick J. Wong wrote:
> <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.

Ah, perfect! Yeah, this is one of the other standard solutions -- header
and flex array handled separately. I'm still working on APIs to handle
the common cases, though. XFS probably will want to keep it separate as
you've done.

> I sent my candidate fixes series to the list just now.

Thanks! I'll go check them out.

-- 
Kees Cook

Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item memcpy

[Dave Chinner]

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@fromorbit.com

Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item memcpy

[Kees Cook]

On Tue, Oct 25, 2022 at 09:32:35AM +1100, Dave Chinner wrote:
> 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?

Right -- this is why I had originally sent my API proposal with the *_dup
helpers included as well. The much more common case is allocate/copy
and allocate/deserialize. The case of doing flex-to-flex is odd, because
it implies there was an external allocation step, etc. But, that said,
allocation and bounds recording are usually pretty well tied together.

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

I think the transition to a fallible routine is an improvement, but
yes, I expect flex_cpy() not to be used much compared to flex_dup(),
or mem_to_flex_dup(), both of which collapse a great many steps and
sanity checks into a single common, internally-consistent, and fallible
operation.

-- 
Kees Cook