diff mbox series

[1/6] xfs: fix validation in attr log item recovery

Message ID 166664715731.2688790.9836328662603103847.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>

Before we start fixing all the complaints about memcpy'ing log items
around, let's fix some inadequate validation in the xattr log item
recovery code and get rid of the (now trivial) copy_format function.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_attr_item.c |   54 ++++++++++++++++++++----------------------------
 1 file changed, 23 insertions(+), 31 deletions(-)

Comments

Kees Cook Oct. 25, 2022, 6:50 p.m. UTC | #1 
On Mon, Oct 24, 2022 at 02:32:37PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Before we start fixing all the complaints about memcpy'ing log items
> around, let's fix some inadequate validation in the xattr log item
> recovery code and get rid of the (now trivial) copy_format function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

I can't speak to the new checks, but yeah, the decomposition to a direct
check and memcpy looks correct.

Reviewed-by: Kees Cook <keescook@chromium.org>
Allison Henderson Oct. 25, 2022, 8:42 p.m. UTC | #2 
On Mon, 2022-10-24 at 14:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Before we start fixing all the complaints about memcpy'ing log items
> around, let's fix some inadequate validation in the xattr log item
> recovery code and get rid of the (now trivial) copy_format function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>\
Ok, looks good to me
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  fs/xfs/xfs_attr_item.c |   54 ++++++++++++++++++++------------------
> ----------
>  1 file changed, 23 insertions(+), 31 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index cf5ce607dc05..ee8f678a10a1 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -245,28 +245,6 @@ xfs_attri_init(
>         return attrip;
>  }
>  
> -/*
> - * Copy an attr format buffer from the given buf, and into the
> destination attr
> - * format structure.
> - */
> -STATIC int
> -xfs_attri_copy_format(
> -       struct xfs_log_iovec            *buf,
> -       struct xfs_attri_log_format     *dst_attr_fmt)
> -{
> -       struct xfs_attri_log_format     *src_attr_fmt = buf->i_addr;
> -       size_t                          len;
> -
> -       len = sizeof(struct xfs_attri_log_format);
> -       if (buf->i_len != len) {
> -               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> -               return -EFSCORRUPTED;
> -       }
> -
> -       memcpy((char *)dst_attr_fmt, (char *)src_attr_fmt, len);
> -       return 0;
> -}
> -
>  static inline struct xfs_attrd_log_item *ATTRD_ITEM(struct
> xfs_log_item *lip)
>  {
>         return container_of(lip, struct xfs_attrd_log_item,
> attrd_item);
> @@ -731,24 +709,44 @@ xlog_recover_attri_commit_pass2(
>         struct xfs_attri_log_nameval    *nv;
>         const void                      *attr_value = NULL;
>         const void                      *attr_name;
> -       int                             error;
> +       size_t                          len;
>  
>         attri_formatp = item->ri_buf[0].i_addr;
>         attr_name = item->ri_buf[1].i_addr;
>  
>         /* Validate xfs_attri_log_format before the large memory
> allocation */
> +       len = sizeof(struct xfs_attri_log_format);
> +       if (item->ri_buf[0].i_len != len) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         if (!xfs_attri_validate(mp, attri_formatp)) {
>                 XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
>                 return -EFSCORRUPTED;
>         }
>  
> +       /* Validate the attr name */
> +       if (item->ri_buf[1].i_len !=
> +                       xlog_calc_iovec_len(attri_formatp-
> >alfi_name_len)) {
> +               XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> +               return -EFSCORRUPTED;
> +       }
> +
>         if (!xfs_attr_namecheck(attr_name, attri_formatp-
> >alfi_name_len)) {
>                 XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
>                 return -EFSCORRUPTED;
>         }
>  
> -       if (attri_formatp->alfi_value_len)
> +       /* Validate the attr value, if present */
> +       if (attri_formatp->alfi_value_len != 0) {
> +               if (item->ri_buf[2].i_len !=
> xlog_calc_iovec_len(attri_formatp->alfi_value_len)) {
> +                       XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> mp);
> +                       return -EFSCORRUPTED;
> +               }
> +
>                 attr_value = item->ri_buf[2].i_addr;
> +       }
>  
>         /*
>          * Memory alloc failure will cause replay to abort.  We
> attach the
> @@ -760,9 +758,7 @@ xlog_recover_attri_commit_pass2(
>                         attri_formatp->alfi_value_len);
>  
>         attrip = xfs_attri_init(mp, nv);
> -       error = xfs_attri_copy_format(&item->ri_buf[0], &attrip-
> >attri_format);
> -       if (error)
> -               goto out;
> +       memcpy(&attrip->attri_format, attri_formatp, len);
>  
>         /*
>          * The ATTRI has two references. One for the ATTRD and one
> for ATTRI to
> @@ -774,10 +770,6 @@ xlog_recover_attri_commit_pass2(
>         xfs_attri_release(attrip);
>         xfs_attri_log_nameval_put(nv);
>         return 0;
> -out:
> -       xfs_attri_item_free(attrip);
> -       xfs_attri_log_nameval_put(nv);
> -       return error;
>  }
>  
>  /*
>
Dave Chinner Oct. 25, 2022, 9:19 p.m. UTC | #3 
On Mon, Oct 24, 2022 at 02:32:37PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Before we start fixing all the complaints about memcpy'ing log items
> around, let's fix some inadequate validation in the xattr log item
> recovery code and get rid of the (now trivial) copy_format function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_attr_item.c |   54 ++++++++++++++++++++----------------------------
>  1 file changed, 23 insertions(+), 31 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index cf5ce607dc05..ee8f678a10a1 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -245,28 +245,6 @@ xfs_attri_init(
>  	return attrip;
>  }
>  
> -/*
> - * Copy an attr format buffer from the given buf, and into the destination attr
> - * format structure.
> - */
> -STATIC int
> -xfs_attri_copy_format(
> -	struct xfs_log_iovec		*buf,
> -	struct xfs_attri_log_format	*dst_attr_fmt)
> -{
> -	struct xfs_attri_log_format	*src_attr_fmt = buf->i_addr;
> -	size_t				len;
> -
> -	len = sizeof(struct xfs_attri_log_format);
> -	if (buf->i_len != len) {
> -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> -		return -EFSCORRUPTED;
> -	}
> -
> -	memcpy((char *)dst_attr_fmt, (char *)src_attr_fmt, len);
> -	return 0;
> -}
> -
>  static inline struct xfs_attrd_log_item *ATTRD_ITEM(struct xfs_log_item *lip)
>  {
>  	return container_of(lip, struct xfs_attrd_log_item, attrd_item);
> @@ -731,24 +709,44 @@ xlog_recover_attri_commit_pass2(
>  	struct xfs_attri_log_nameval	*nv;
>  	const void			*attr_value = NULL;
>  	const void			*attr_name;
> -	int                             error;
> +	size_t				len;
>  
>  	attri_formatp = item->ri_buf[0].i_addr;
>  	attr_name = item->ri_buf[1].i_addr;
>  
>  	/* Validate xfs_attri_log_format before the large memory allocation */
> +	len = sizeof(struct xfs_attri_log_format);
> +	if (item->ri_buf[0].i_len != len) {
> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> +		return -EFSCORRUPTED;
> +	}

I can't help but think these should use XFS_CORRPUPTION_ERROR() so
that we get a dump of the corrupt log format structure along with
error message.

Regardless, the change looks good - validating the name/value region
sizes before we allocate and copy them is a good idea. :)

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Darrick J. Wong Oct. 25, 2022, 10:05 p.m. UTC | #4 
On Wed, Oct 26, 2022 at 08:19:27AM +1100, Dave Chinner wrote:
> On Mon, Oct 24, 2022 at 02:32:37PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Before we start fixing all the complaints about memcpy'ing log items
> > around, let's fix some inadequate validation in the xattr log item
> > recovery code and get rid of the (now trivial) copy_format function.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_attr_item.c |   54 ++++++++++++++++++++----------------------------
> >  1 file changed, 23 insertions(+), 31 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index cf5ce607dc05..ee8f678a10a1 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -245,28 +245,6 @@ xfs_attri_init(
> >  	return attrip;
> >  }
> >  
> > -/*
> > - * Copy an attr format buffer from the given buf, and into the destination attr
> > - * format structure.
> > - */
> > -STATIC int
> > -xfs_attri_copy_format(
> > -	struct xfs_log_iovec		*buf,
> > -	struct xfs_attri_log_format	*dst_attr_fmt)
> > -{
> > -	struct xfs_attri_log_format	*src_attr_fmt = buf->i_addr;
> > -	size_t				len;
> > -
> > -	len = sizeof(struct xfs_attri_log_format);
> > -	if (buf->i_len != len) {
> > -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> > -		return -EFSCORRUPTED;
> > -	}
> > -
> > -	memcpy((char *)dst_attr_fmt, (char *)src_attr_fmt, len);
> > -	return 0;
> > -}
> > -
> >  static inline struct xfs_attrd_log_item *ATTRD_ITEM(struct xfs_log_item *lip)
> >  {
> >  	return container_of(lip, struct xfs_attrd_log_item, attrd_item);
> > @@ -731,24 +709,44 @@ xlog_recover_attri_commit_pass2(
> >  	struct xfs_attri_log_nameval	*nv;
> >  	const void			*attr_value = NULL;
> >  	const void			*attr_name;
> > -	int                             error;
> > +	size_t				len;
> >  
> >  	attri_formatp = item->ri_buf[0].i_addr;
> >  	attr_name = item->ri_buf[1].i_addr;
> >  
> >  	/* Validate xfs_attri_log_format before the large memory allocation */
> > +	len = sizeof(struct xfs_attri_log_format);
> > +	if (item->ri_buf[0].i_len != len) {
> > +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> > +		return -EFSCORRUPTED;
> > +	}
> 
> I can't help but think these should use XFS_CORRPUPTION_ERROR() so
> that we get a dump of the corrupt log format structure along with
> error message.

That is a good idea.  I will tack a new patch on the end to make that
conversion.

--D

> Regardless, the change looks good - validating the name/value region
> sizes before we allocate and copy them is a good idea. :)
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index cf5ce607dc05..ee8f678a10a1 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -245,28 +245,6 @@  xfs_attri_init(
 	return attrip;
 }
 
-/*
- * Copy an attr format buffer from the given buf, and into the destination attr
- * format structure.
- */
-STATIC int
-xfs_attri_copy_format(
-	struct xfs_log_iovec		*buf,
-	struct xfs_attri_log_format	*dst_attr_fmt)
-{
-	struct xfs_attri_log_format	*src_attr_fmt = buf->i_addr;
-	size_t				len;
-
-	len = sizeof(struct xfs_attri_log_format);
-	if (buf->i_len != len) {
-		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
-		return -EFSCORRUPTED;
-	}
-
-	memcpy((char *)dst_attr_fmt, (char *)src_attr_fmt, len);
-	return 0;
-}
-
 static inline struct xfs_attrd_log_item *ATTRD_ITEM(struct xfs_log_item *lip)
 {
 	return container_of(lip, struct xfs_attrd_log_item, attrd_item);
@@ -731,24 +709,44 @@  xlog_recover_attri_commit_pass2(
 	struct xfs_attri_log_nameval	*nv;
 	const void			*attr_value = NULL;
 	const void			*attr_name;
-	int                             error;
+	size_t				len;
 
 	attri_formatp = item->ri_buf[0].i_addr;
 	attr_name = item->ri_buf[1].i_addr;
 
 	/* Validate xfs_attri_log_format before the large memory allocation */
+	len = sizeof(struct xfs_attri_log_format);
+	if (item->ri_buf[0].i_len != len) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
+		return -EFSCORRUPTED;
+	}
+
 	if (!xfs_attri_validate(mp, attri_formatp)) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
 		return -EFSCORRUPTED;
 	}
 
+	/* Validate the attr name */
+	if (item->ri_buf[1].i_len !=
+			xlog_calc_iovec_len(attri_formatp->alfi_name_len)) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
+		return -EFSCORRUPTED;
+	}
+
 	if (!xfs_attr_namecheck(attr_name, attri_formatp->alfi_name_len)) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
 		return -EFSCORRUPTED;
 	}
 
-	if (attri_formatp->alfi_value_len)
+	/* Validate the attr value, if present */
+	if (attri_formatp->alfi_value_len != 0) {
+		if (item->ri_buf[2].i_len != xlog_calc_iovec_len(attri_formatp->alfi_value_len)) {
+			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
+			return -EFSCORRUPTED;
+		}
+
 		attr_value = item->ri_buf[2].i_addr;
+	}
 
 	/*
 	 * Memory alloc failure will cause replay to abort.  We attach the
@@ -760,9 +758,7 @@  xlog_recover_attri_commit_pass2(
 			attri_formatp->alfi_value_len);
 
 	attrip = xfs_attri_init(mp, nv);
-	error = xfs_attri_copy_format(&item->ri_buf[0], &attrip->attri_format);
-	if (error)
-		goto out;
+	memcpy(&attrip->attri_format, attri_formatp, len);
 
 	/*
 	 * The ATTRI has two references. One for the ATTRD and one for ATTRI to
@@ -774,10 +770,6 @@  xlog_recover_attri_commit_pass2(
 	xfs_attri_release(attrip);
 	xfs_attri_log_nameval_put(nv);
 	return 0;
-out:
-	xfs_attri_item_free(attrip);
-	xfs_attri_log_nameval_put(nv);
-	return error;
 }
 
 /*