diff mbox series

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

Message ID 166664716290.2688790.11246233896801948595.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: fix various problems with log intent item recovery | expand

Commit Message

Darrick J. Wong Oct. 24, 2022, 9:32 p.m. UTC 
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(-)

Comments

Kees Cook Oct. 25, 2022, 6:52 p.m. UTC | #1 
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. :)
Allison Henderson Oct. 25, 2022, 8:47 p.m. UTC | #2 
On Mon, 2022-10-24 at 14:32 -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>
Ok, makes sense:
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  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));
>  
> -       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));
>  }
>  
>  /*
> @@ -646,23 +636,31 @@ xlog_recover_bui_commit_pass2(
>         struct xlog_recover_item        *item,
>         xfs_lsn_t                       lsn)
>  {
> -       int                             error;
>         struct xfs_mount                *mp = log->l_mp;
>         struct xfs_bui_log_item         *buip;
>         struct xfs_bui_log_format       *bui_formatp;
> +       size_t                          len;
>  
>         bui_formatp = item->ri_buf[0].i_addr;
>  
> +       if (item->ri_buf[0].i_len < xfs_bui_log_format_sizeof(0)) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         if (bui_formatp->bui_nextents != XFS_BUI_MAX_FAST_EXTENTS) {
>                 XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
>                 return -EFSCORRUPTED;
>         }
> +
> +       len = xfs_bui_log_format_sizeof(bui_formatp->bui_nextents);
> +       if (item->ri_buf[0].i_len != len) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         buip = xfs_bui_init(mp);
> -       error = xfs_bui_copy_format(&item->ri_buf[0], &buip-
> >bui_format);
> -       if (error) {
> -               xfs_bui_item_free(buip);
> -               return error;
> -       }
> +       xfs_bui_copy_format(&buip->bui_format, bui_formatp);
>         atomic_set(&buip->bui_next_extent, bui_formatp-
> >bui_nextents);
>         /*
>          * Insert the intent into the AIL directly and drop one
> reference so
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 758702b9495f..56917e236370 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -134,6 +134,11 @@ xfs_check_ondisk_structs(void)
>         XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header,          16);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_attri_log_format,      40);
>         XFS_CHECK_STRUCT_SIZE(struct xfs_attrd_log_format,      16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_bui_log_format,        16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_bud_log_format,        16);
> +       XFS_CHECK_STRUCT_SIZE(struct xfs_map_extent,            32);
> +
> +       XFS_CHECK_OFFSET(struct xfs_bui_log_format,
> bui_extents,        16);
>  
>         /*
>          * The v5 superblock format extended several v4 header
> structures with
>
Dave Chinner Oct. 25, 2022, 9:34 p.m. UTC | #3 
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>

Looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
diff mbox series

Patch

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));
 
-	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));
 }
 
 /*
@@ -646,23 +636,31 @@  xlog_recover_bui_commit_pass2(
 	struct xlog_recover_item	*item,
 	xfs_lsn_t			lsn)
 {
-	int				error;
 	struct xfs_mount		*mp = log->l_mp;
 	struct xfs_bui_log_item		*buip;
 	struct xfs_bui_log_format	*bui_formatp;
+	size_t				len;
 
 	bui_formatp = item->ri_buf[0].i_addr;
 
+	if (item->ri_buf[0].i_len < xfs_bui_log_format_sizeof(0)) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		return -EFSCORRUPTED;
+	}
+
 	if (bui_formatp->bui_nextents != XFS_BUI_MAX_FAST_EXTENTS) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
 		return -EFSCORRUPTED;
 	}
+
+	len = xfs_bui_log_format_sizeof(bui_formatp->bui_nextents);
+	if (item->ri_buf[0].i_len != len) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp);
+		return -EFSCORRUPTED;
+	}
+
 	buip = xfs_bui_init(mp);
-	error = xfs_bui_copy_format(&item->ri_buf[0], &buip->bui_format);
-	if (error) {
-		xfs_bui_item_free(buip);
-		return error;
-	}
+	xfs_bui_copy_format(&buip->bui_format, bui_formatp);
 	atomic_set(&buip->bui_next_extent, bui_formatp->bui_nextents);
 	/*
 	 * Insert the intent into the AIL directly and drop one reference so
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index 758702b9495f..56917e236370 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -134,6 +134,11 @@  xfs_check_ondisk_structs(void)
 	XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header,		16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_attri_log_format,	40);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_attrd_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_bui_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_bud_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_map_extent,		32);
+
+	XFS_CHECK_OFFSET(struct xfs_bui_log_format, bui_extents,	16);
 
 	/*
 	 * The v5 superblock format extended several v4 header structures with