On Mon, 2022-10-24 at 14:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>\
Ok, looks good to me
Reviewed-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> ---
> 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;
> }
>
> /*
>
