Message ID | 166664715731.2688790.9836328662603103847.stgit@magnolia (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | xfs: fix various problems with log intent item recovery | expand |
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> I can't speak to the new checks, but yeah, the decomposition to a direct check and memcpy looks correct. Reviewed-by: Kees Cook <keescook@chromium.org>
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; > } > > /* >
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. 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>
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
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; } /*