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

[Allison Henderson <allison.henderson@oracle.com>]

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;
>  }
>  
>  /*
>