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@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.
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@xxxxxxxxxx>
>
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
