Re: [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying

[Kees Cook <keescook@chromium.org>]

On Mon, Oct 24, 2022 at 02:32:42PM -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 flex arrays correctly:
> 
> ------------[ cut here ]------------
> memcpy: detected field-spanning write (size 48) of single field "dst_bui_fmt" at fs/xfs/xfs_bmap_item.c:628 (size 16)
> 
> Fix this by refactoring the xfs_bui_copy_format function to handle the
> copying of the head and the flex array members separately.  While we're
> at it, fix a minor validation deficiency in the recovery function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_bmap_item.c |   46 ++++++++++++++++++++++------------------------
>  fs/xfs/xfs_ondisk.h    |    5 +++++
>  2 files changed, 27 insertions(+), 24 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 51f66e982484..a1da6205252b 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -608,28 +608,18 @@ static const struct xfs_item_ops xfs_bui_item_ops = {
>  	.iop_relog	= xfs_bui_item_relog,
>  };
>  
> -/*
> - * 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
> +static inline void
>  xfs_bui_copy_format(
> -	struct xfs_log_iovec		*buf,
> -	struct xfs_bui_log_format	*dst_bui_fmt)
> +	struct xfs_bui_log_format	*dst,
> +	const struct xfs_bui_log_format	*src)
>  {
> -	struct xfs_bui_log_format	*src_bui_fmt;
> -	uint				len;
> +	unsigned int			i;
>  
> -	src_bui_fmt = buf->i_addr;
> -	len = xfs_bui_log_format_sizeof(src_bui_fmt->bui_nextents);
> +	memcpy(dst, src, offsetof(struct xfs_bui_log_format, bui_extents));

I think this would work:

	*dst = *src;

>  
> -	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;
> +	for (i = 0; i < src->bui_nextents; i++)
> +		memcpy(&dst->bui_extents[i], &src->bui_extents[i],
> +				sizeof(struct xfs_map_extent));

Same here:

		dst->bui_extents[i] = src->bui_extents[i];

No reason to bring memcpy into this at all. :)


-- 
Kees Cook