On Mon, Oct 24, 2022 at 02:32:37PM -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>
> ---
> 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@xxxxxxxxxx>
--
Dave Chinner
david@xxxxxxxxxxxxx
