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