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

[Kees Cook <keescook@chromium.org>]

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