From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/6] xfs: fix validation in attr log item recovery
Date: Tue, 25 Oct 2022 15:05:21 -0700 [thread overview]
Message-ID: <Y1hdoXzYppP6bG58@magnolia> (raw)
In-Reply-To: <20221025211927.GB3600936@dread.disaster.area>
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
next prev parent reply other threads:[~2022-10-25 22:05 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-24 21:32 [PATCHSET 0/6] xfs: fix various problems with log intent item recovery Darrick J. Wong
2022-10-24 21:32 ` [PATCH 1/6] xfs: fix validation in attr log " Darrick J. Wong
2022-10-25 18:50 ` Kees Cook
2022-10-25 20:42 ` Allison Henderson
2022-10-25 21:19 ` Dave Chinner
2022-10-25 22:05 ` Darrick J. Wong [this message]
2022-10-24 21:32 ` [PATCH 2/6] xfs: fix memcpy fortify errors in BUI log format copying Darrick J. Wong
2022-10-25 18:52 ` Kees Cook
2022-10-25 20:47 ` Allison Henderson
2022-10-25 21:34 ` Dave Chinner
2022-10-24 21:32 ` [PATCH 3/6] xfs: fix memcpy fortify errors in CUI " Darrick J. Wong
2022-10-25 20:47 ` Allison Henderson
2022-10-25 21:36 ` Dave Chinner
2022-10-24 21:32 ` [PATCH 4/6] xfs: fix memcpy fortify errors in RUI " Darrick J. Wong
2022-10-25 20:49 ` Allison Henderson
2022-10-25 21:37 ` Dave Chinner
2022-10-24 21:32 ` [PATCH 5/6] xfs: fix memcpy fortify errors in EFI " Darrick J. Wong
2022-10-25 19:08 ` Kees Cook
2022-10-25 20:54 ` Allison Henderson
2022-10-25 21:17 ` Darrick J. Wong
2022-10-25 21:47 ` Dave Chinner
2022-10-24 21:33 ` [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic Darrick J. Wong
2022-10-25 19:14 ` Kees Cook
2022-10-25 20:56 ` Allison Henderson
2022-10-25 22:05 ` Dave Chinner
2022-10-25 22:08 ` Darrick J. Wong
2022-10-25 22:22 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y1hdoXzYppP6bG58@magnolia \
--to=djwong@kernel.org \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox