| Message ID | 166664717980.2688790.14877643421674738495.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: fix various problems with log intent item recovery | expand |
On Mon, Oct 24, 2022 at 02:32:59PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of > memcpy. Since we're already fixing problems with BUI item copying, we > should fix it everything else. > > An extra difficulty here is that the ef[id]_extents arrays are declared > as single-element arrays. This is not the convention for flex arrays in > the modern kernel, and it causes all manner of problems with static > checking tools, since they often cannot tell the difference between a > single element array and a flex array. > > So for starters, change those array[1] declarations to array[] > declarations to signal that they are proper flex arrays and adjust all > the "size-1" expressions to fit the new declaration style. This looks very familiar! :) https://lore.kernel.org/linux-xfs/20210419082804.2076124-1-hch@lst.de/ It seems like it might make more sense to start with hch's series, and see what's missing? > > Next, refactor the xfs_efi_copy_format function to handle the copying of > the head and the flex array members separately. While we're at it, fix > a minor validation deficiency in the recovery function. This feels like 3 separate logical changes in a single patch, but, regardless: Reviewed-by: Kees Cook <keescook@chromium.org> This will proactively fix XFS under CONFIG_UBSAN_BOUNDS once -fstrict-flex-arrays is added. Thank you!
On Mon, 2022-10-24 at 14:32 -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of > memcpy. Since we're already fixing problems with BUI item copying, > we > should fix it everything else. > > An extra difficulty here is that the ef[id]_extents arrays are > declared > as single-element arrays. This is not the convention for flex arrays > in > the modern kernel, and it causes all manner of problems with static > checking tools, since they often cannot tell the difference between a > single element array and a flex array. > > So for starters, change those array[1] declarations to array[] > declarations to signal that they are proper flex arrays and adjust > all > the "size-1" expressions to fit the new declaration style. > > Next, refactor the xfs_efi_copy_format function to handle the copying > of > the head and the flex array members separately. While we're at it, > fix > a minor validation deficiency in the recovery function. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_log_format.h | 12 ++++++------ > fs/xfs/xfs_extfree_item.c | 31 +++++++++++++++++++++--------- > - > fs/xfs/xfs_ondisk.h | 11 +++++++---- > fs/xfs/xfs_super.c | 4 ++-- > 4 files changed, 36 insertions(+), 22 deletions(-) > > > diff --git a/fs/xfs/libxfs/xfs_log_format.h > b/fs/xfs/libxfs/xfs_log_format.h > index b351b9dc6561..2f41fa8477c9 100644 > --- a/fs/xfs/libxfs/xfs_log_format.h > +++ b/fs/xfs/libxfs/xfs_log_format.h > @@ -613,7 +613,7 @@ typedef struct xfs_efi_log_format { > uint16_t efi_size; /* size of this item > */ > uint32_t efi_nextents; /* # extents to free > */ > uint64_t efi_id; /* efi identifier */ > - xfs_extent_t efi_extents[1]; /* array of extents > to free */ > + xfs_extent_t efi_extents[]; /* array of extents > to free */ > } xfs_efi_log_format_t; > > typedef struct xfs_efi_log_format_32 { > @@ -621,7 +621,7 @@ typedef struct xfs_efi_log_format_32 { > uint16_t efi_size; /* size of this item > */ > uint32_t efi_nextents; /* # extents to free > */ > uint64_t efi_id; /* efi identifier */ > - xfs_extent_32_t efi_extents[1]; /* array of extents > to free */ > + xfs_extent_32_t efi_extents[]; /* array of extents > to free */ > } __attribute__((packed)) xfs_efi_log_format_32_t; > > typedef struct xfs_efi_log_format_64 { > @@ -629,7 +629,7 @@ typedef struct xfs_efi_log_format_64 { > uint16_t efi_size; /* size of this item > */ > uint32_t efi_nextents; /* # extents to free > */ > uint64_t efi_id; /* efi identifier */ > - xfs_extent_64_t efi_extents[1]; /* array of extents > to free */ > + xfs_extent_64_t efi_extents[]; /* array of extents > to free */ > } xfs_efi_log_format_64_t; > > /* > @@ -642,7 +642,7 @@ typedef struct xfs_efd_log_format { > uint16_t efd_size; /* size of this item > */ > uint32_t efd_nextents; /* # of extents freed > */ > uint64_t efd_efi_id; /* id of > corresponding efi */ > - xfs_extent_t efd_extents[1]; /* array of extents > freed */ > + xfs_extent_t efd_extents[]; /* array of extents > freed */ > } xfs_efd_log_format_t; > > typedef struct xfs_efd_log_format_32 { > @@ -650,7 +650,7 @@ typedef struct xfs_efd_log_format_32 { > uint16_t efd_size; /* size of this item > */ > uint32_t efd_nextents; /* # of extents freed > */ > uint64_t efd_efi_id; /* id of > corresponding efi */ > - xfs_extent_32_t efd_extents[1]; /* array of extents > freed */ > + xfs_extent_32_t efd_extents[]; /* array of extents > freed */ > } __attribute__((packed)) xfs_efd_log_format_32_t; > > typedef struct xfs_efd_log_format_64 { > @@ -658,7 +658,7 @@ typedef struct xfs_efd_log_format_64 { > uint16_t efd_size; /* size of this item > */ > uint32_t efd_nextents; /* # of extents freed > */ > uint64_t efd_efi_id; /* id of > corresponding efi */ > - xfs_extent_64_t efd_extents[1]; /* array of extents > freed */ > + xfs_extent_64_t efd_extents[]; /* array of extents > freed */ > } xfs_efd_log_format_64_t; > > /* > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c > index 27ccfcd82f04..466cc5c5cd33 100644 > --- a/fs/xfs/xfs_extfree_item.c > +++ b/fs/xfs/xfs_extfree_item.c > @@ -76,7 +76,7 @@ xfs_efi_item_sizeof( > struct xfs_efi_log_item *efip) > { > return sizeof(struct xfs_efi_log_format) + > - (efip->efi_format.efi_nextents - 1) * > sizeof(xfs_extent_t); > + efip->efi_format.efi_nextents * sizeof(xfs_extent_t); Did we want to try and avoid using typedefs? I notice that seems to come up a lot in reviews. Otherwise the rest looks good. Allison > } > > STATIC void > @@ -160,7 +160,7 @@ xfs_efi_init( > ASSERT(nextents > 0); > if (nextents > XFS_EFI_MAX_FAST_EXTENTS) { > size = (uint)(sizeof(struct xfs_efi_log_item) + > - ((nextents - 1) * sizeof(xfs_extent_t))); > + (nextents * sizeof(xfs_extent_t))); > efip = kmem_zalloc(size, 0); > } else { > efip = kmem_cache_zalloc(xfs_efi_cache, > @@ -189,14 +189,19 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf, > xfs_efi_log_format_t *dst_efi_fmt) > xfs_efi_log_format_t *src_efi_fmt = buf->i_addr; > uint i; > uint len = sizeof(xfs_efi_log_format_t) + > - (src_efi_fmt->efi_nextents - 1) * > sizeof(xfs_extent_t); > + src_efi_fmt->efi_nextents * sizeof(xfs_extent_t); > uint len32 = sizeof(xfs_efi_log_format_32_t) + > - (src_efi_fmt->efi_nextents - 1) * > sizeof(xfs_extent_32_t); > + src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t); > uint len64 = sizeof(xfs_efi_log_format_64_t) + > - (src_efi_fmt->efi_nextents - 1) * > sizeof(xfs_extent_64_t); > + src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t); > > if (buf->i_len == len) { > - memcpy((char *)dst_efi_fmt, (char*)src_efi_fmt, len); > + memcpy(dst_efi_fmt, src_efi_fmt, > + offsetof(struct xfs_efi_log_format, > efi_extents)); > + for (i = 0; i < src_efi_fmt->efi_nextents; i++) > + memcpy(&dst_efi_fmt->efi_extents[i], > + &src_efi_fmt->efi_extents[i], > + sizeof(struct xfs_extent)); > return 0; > } else if (buf->i_len == len32) { > xfs_efi_log_format_32_t *src_efi_fmt_32 = buf- > >i_addr; > @@ -256,7 +261,7 @@ xfs_efd_item_sizeof( > struct xfs_efd_log_item *efdp) > { > return sizeof(xfs_efd_log_format_t) + > - (efdp->efd_format.efd_nextents - 1) * > sizeof(xfs_extent_t); > + efdp->efd_format.efd_nextents * sizeof(xfs_extent_t); > } > > STATIC void > @@ -341,7 +346,7 @@ xfs_trans_get_efd( > > if (nextents > XFS_EFD_MAX_FAST_EXTENTS) { > efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) + > - (nextents - 1) * sizeof(struct > xfs_extent), > + nextents * sizeof(struct xfs_extent), > 0); > } else { > efdp = kmem_cache_zalloc(xfs_efd_cache, > @@ -733,6 +738,12 @@ xlog_recover_efi_commit_pass2( > > efi_formatp = item->ri_buf[0].i_addr; > > + if (item->ri_buf[0].i_len < > + offsetof(struct xfs_efi_log_format, > efi_extents)) { > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log- > >l_mp); > + return -EFSCORRUPTED; > + } > + > efip = xfs_efi_init(mp, efi_formatp->efi_nextents); > error = xfs_efi_copy_format(&item->ri_buf[0], &efip- > >efi_format); > if (error) { > @@ -772,9 +783,9 @@ xlog_recover_efd_commit_pass2( > > efd_formatp = item->ri_buf[0].i_addr; > ASSERT((item->ri_buf[0].i_len == > (sizeof(xfs_efd_log_format_32_t) + > - ((efd_formatp->efd_nextents - 1) * > sizeof(xfs_extent_32_t)))) || > + (efd_formatp->efd_nextents * > sizeof(xfs_extent_32_t)))) || > (item->ri_buf[0].i_len == > (sizeof(xfs_efd_log_format_64_t) + > - ((efd_formatp->efd_nextents - 1) * > sizeof(xfs_extent_64_t))))); > + (efd_formatp->efd_nextents * > sizeof(xfs_extent_64_t))))); > > xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp- > >efd_efi_id); > return 0; > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h > index 19c1df00b48e..9737b5a9f405 100644 > --- a/fs/xfs/xfs_ondisk.h > +++ b/fs/xfs/xfs_ondisk.h > @@ -118,10 +118,10 @@ xfs_check_ondisk_structs(void) > /* log structures */ > XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format, 88); > XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat, 24); > - XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32, 28); > - XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64, 32); > - XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32, 28); > - XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64, 32); > + XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32, 16); > + XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64, 16); > + XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32, 16); > + XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64, 16); > XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32, 12); > XFS_CHECK_STRUCT_SIZE(struct xfs_extent_64, 16); > XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode, 176); > @@ -146,6 +146,9 @@ xfs_check_ondisk_structs(void) > XFS_CHECK_OFFSET(struct xfs_bui_log_format, > bui_extents, 16); > XFS_CHECK_OFFSET(struct xfs_cui_log_format, > cui_extents, 16); > XFS_CHECK_OFFSET(struct xfs_rui_log_format, > rui_extents, 16); > + XFS_CHECK_OFFSET(struct xfs_efi_log_format, > efi_extents, 16); > + XFS_CHECK_OFFSET(struct xfs_efi_log_format_32, > efi_extents, 16); > + XFS_CHECK_OFFSET(struct xfs_efi_log_format_64, > efi_extents, 16); > > /* > * The v5 superblock format extended several v4 header > structures with > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index f029c6702dda..8485e3b37ca0 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -2029,7 +2029,7 @@ xfs_init_caches(void) > > xfs_efd_cache = kmem_cache_create("xfs_efd_item", > (sizeof(struct > xfs_efd_log_item) + > - (XFS_EFD_MAX_FAST_EXTENTS - > 1) * > + XFS_EFD_MAX_FAST_EXTENTS * > sizeof(struct xfs_extent)), > 0, 0, NULL); > if (!xfs_efd_cache) > @@ -2037,7 +2037,7 @@ xfs_init_caches(void) > > xfs_efi_cache = kmem_cache_create("xfs_efi_item", > (sizeof(struct > xfs_efi_log_item) + > - (XFS_EFI_MAX_FAST_EXTENTS - > 1) * > + XFS_EFI_MAX_FAST_EXTENTS * > sizeof(struct xfs_extent)), > 0, 0, NULL); > if (!xfs_efi_cache) >
On Tue, Oct 25, 2022 at 08:54:25PM +0000, Allison Henderson wrote: > On Mon, 2022-10-24 at 14:32 -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of > > memcpy. Since we're already fixing problems with BUI item copying, > > we > > should fix it everything else. > > > > An extra difficulty here is that the ef[id]_extents arrays are > > declared > > as single-element arrays. This is not the convention for flex arrays > > in > > the modern kernel, and it causes all manner of problems with static > > checking tools, since they often cannot tell the difference between a > > single element array and a flex array. > > > > So for starters, change those array[1] declarations to array[] > > declarations to signal that they are proper flex arrays and adjust > > all > > the "size-1" expressions to fit the new declaration style. > > > > Next, refactor the xfs_efi_copy_format function to handle the copying > > of > > the head and the flex array members separately. While we're at it, > > fix > > a minor validation deficiency in the recovery function. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > fs/xfs/libxfs/xfs_log_format.h | 12 ++++++------ > > fs/xfs/xfs_extfree_item.c | 31 +++++++++++++++++++++--------- > > - > > fs/xfs/xfs_ondisk.h | 11 +++++++---- > > fs/xfs/xfs_super.c | 4 ++-- > > 4 files changed, 36 insertions(+), 22 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_log_format.h > > b/fs/xfs/libxfs/xfs_log_format.h > > index b351b9dc6561..2f41fa8477c9 100644 > > --- a/fs/xfs/libxfs/xfs_log_format.h > > +++ b/fs/xfs/libxfs/xfs_log_format.h > > @@ -613,7 +613,7 @@ typedef struct xfs_efi_log_format { > > uint16_t efi_size; /* size of this item > > */ > > uint32_t efi_nextents; /* # extents to free > > */ > > uint64_t efi_id; /* efi identifier */ > > - xfs_extent_t efi_extents[1]; /* array of extents > > to free */ > > + xfs_extent_t efi_extents[]; /* array of extents > > to free */ > > } xfs_efi_log_format_t; > > > > typedef struct xfs_efi_log_format_32 { > > @@ -621,7 +621,7 @@ typedef struct xfs_efi_log_format_32 { > > uint16_t efi_size; /* size of this item > > */ > > uint32_t efi_nextents; /* # extents to free > > */ > > uint64_t efi_id; /* efi identifier */ > > - xfs_extent_32_t efi_extents[1]; /* array of extents > > to free */ > > + xfs_extent_32_t efi_extents[]; /* array of extents > > to free */ > > } __attribute__((packed)) xfs_efi_log_format_32_t; > > > > typedef struct xfs_efi_log_format_64 { > > @@ -629,7 +629,7 @@ typedef struct xfs_efi_log_format_64 { > > uint16_t efi_size; /* size of this item > > */ > > uint32_t efi_nextents; /* # extents to free > > */ > > uint64_t efi_id; /* efi identifier */ > > - xfs_extent_64_t efi_extents[1]; /* array of extents > > to free */ > > + xfs_extent_64_t efi_extents[]; /* array of extents > > to free */ > > } xfs_efi_log_format_64_t; > > > > /* > > @@ -642,7 +642,7 @@ typedef struct xfs_efd_log_format { > > uint16_t efd_size; /* size of this item > > */ > > uint32_t efd_nextents; /* # of extents freed > > */ > > uint64_t efd_efi_id; /* id of > > corresponding efi */ > > - xfs_extent_t efd_extents[1]; /* array of extents > > freed */ > > + xfs_extent_t efd_extents[]; /* array of extents > > freed */ > > } xfs_efd_log_format_t; > > > > typedef struct xfs_efd_log_format_32 { > > @@ -650,7 +650,7 @@ typedef struct xfs_efd_log_format_32 { > > uint16_t efd_size; /* size of this item > > */ > > uint32_t efd_nextents; /* # of extents freed > > */ > > uint64_t efd_efi_id; /* id of > > corresponding efi */ > > - xfs_extent_32_t efd_extents[1]; /* array of extents > > freed */ > > + xfs_extent_32_t efd_extents[]; /* array of extents > > freed */ > > } __attribute__((packed)) xfs_efd_log_format_32_t; > > > > typedef struct xfs_efd_log_format_64 { > > @@ -658,7 +658,7 @@ typedef struct xfs_efd_log_format_64 { > > uint16_t efd_size; /* size of this item > > */ > > uint32_t efd_nextents; /* # of extents freed > > */ > > uint64_t efd_efi_id; /* id of > > corresponding efi */ > > - xfs_extent_64_t efd_extents[1]; /* array of extents > > freed */ > > + xfs_extent_64_t efd_extents[]; /* array of extents > > freed */ > > } xfs_efd_log_format_64_t; > > > > /* > > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c > > index 27ccfcd82f04..466cc5c5cd33 100644 > > --- a/fs/xfs/xfs_extfree_item.c > > +++ b/fs/xfs/xfs_extfree_item.c > > @@ -76,7 +76,7 @@ xfs_efi_item_sizeof( > > struct xfs_efi_log_item *efip) > > { > > return sizeof(struct xfs_efi_log_format) + > > - (efip->efi_format.efi_nextents - 1) * > > sizeof(xfs_extent_t); > > + efip->efi_format.efi_nextents * sizeof(xfs_extent_t); > Did we want to try and avoid using typedefs? I notice that seems to > come up a lot in reviews. Otherwise the rest looks good. Yes, but I kill off these typedef usages in the next patch by creating the sizeof helpers. --D > Allison > > > } > > > > STATIC void > > @@ -160,7 +160,7 @@ xfs_efi_init( > > ASSERT(nextents > 0); > > if (nextents > XFS_EFI_MAX_FAST_EXTENTS) { > > size = (uint)(sizeof(struct xfs_efi_log_item) + > > - ((nextents - 1) * sizeof(xfs_extent_t))); > > + (nextents * sizeof(xfs_extent_t))); > > efip = kmem_zalloc(size, 0); > > } else { > > efip = kmem_cache_zalloc(xfs_efi_cache, > > @@ -189,14 +189,19 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf, > > xfs_efi_log_format_t *dst_efi_fmt) > > xfs_efi_log_format_t *src_efi_fmt = buf->i_addr; > > uint i; > > uint len = sizeof(xfs_efi_log_format_t) + > > - (src_efi_fmt->efi_nextents - 1) * > > sizeof(xfs_extent_t); > > + src_efi_fmt->efi_nextents * sizeof(xfs_extent_t); > > uint len32 = sizeof(xfs_efi_log_format_32_t) + > > - (src_efi_fmt->efi_nextents - 1) * > > sizeof(xfs_extent_32_t); > > + src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t); > > uint len64 = sizeof(xfs_efi_log_format_64_t) + > > - (src_efi_fmt->efi_nextents - 1) * > > sizeof(xfs_extent_64_t); > > + src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t); > > > > if (buf->i_len == len) { > > - memcpy((char *)dst_efi_fmt, (char*)src_efi_fmt, len); > > + memcpy(dst_efi_fmt, src_efi_fmt, > > + offsetof(struct xfs_efi_log_format, > > efi_extents)); > > + for (i = 0; i < src_efi_fmt->efi_nextents; i++) > > + memcpy(&dst_efi_fmt->efi_extents[i], > > + &src_efi_fmt->efi_extents[i], > > + sizeof(struct xfs_extent)); > > return 0; > > } else if (buf->i_len == len32) { > > xfs_efi_log_format_32_t *src_efi_fmt_32 = buf- > > >i_addr; > > @@ -256,7 +261,7 @@ xfs_efd_item_sizeof( > > struct xfs_efd_log_item *efdp) > > { > > return sizeof(xfs_efd_log_format_t) + > > - (efdp->efd_format.efd_nextents - 1) * > > sizeof(xfs_extent_t); > > + efdp->efd_format.efd_nextents * sizeof(xfs_extent_t); > > } > > > > STATIC void > > @@ -341,7 +346,7 @@ xfs_trans_get_efd( > > > > if (nextents > XFS_EFD_MAX_FAST_EXTENTS) { > > efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) + > > - (nextents - 1) * sizeof(struct > > xfs_extent), > > + nextents * sizeof(struct xfs_extent), > > 0); > > } else { > > efdp = kmem_cache_zalloc(xfs_efd_cache, > > @@ -733,6 +738,12 @@ xlog_recover_efi_commit_pass2( > > > > efi_formatp = item->ri_buf[0].i_addr; > > > > + if (item->ri_buf[0].i_len < > > + offsetof(struct xfs_efi_log_format, > > efi_extents)) { > > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log- > > >l_mp); > > + return -EFSCORRUPTED; > > + } > > + > > efip = xfs_efi_init(mp, efi_formatp->efi_nextents); > > error = xfs_efi_copy_format(&item->ri_buf[0], &efip- > > >efi_format); > > if (error) { > > @@ -772,9 +783,9 @@ xlog_recover_efd_commit_pass2( > > > > efd_formatp = item->ri_buf[0].i_addr; > > ASSERT((item->ri_buf[0].i_len == > > (sizeof(xfs_efd_log_format_32_t) + > > - ((efd_formatp->efd_nextents - 1) * > > sizeof(xfs_extent_32_t)))) || > > + (efd_formatp->efd_nextents * > > sizeof(xfs_extent_32_t)))) || > > (item->ri_buf[0].i_len == > > (sizeof(xfs_efd_log_format_64_t) + > > - ((efd_formatp->efd_nextents - 1) * > > sizeof(xfs_extent_64_t))))); > > + (efd_formatp->efd_nextents * > > sizeof(xfs_extent_64_t))))); > > > > xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp- > > >efd_efi_id); > > return 0; > > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h > > index 19c1df00b48e..9737b5a9f405 100644 > > --- a/fs/xfs/xfs_ondisk.h > > +++ b/fs/xfs/xfs_ondisk.h > > @@ -118,10 +118,10 @@ xfs_check_ondisk_structs(void) > > /* log structures */ > > XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format, 88); > > XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat, 24); > > - XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32, 28); > > - XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64, 32); > > - XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32, 28); > > - XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64, 32); > > + XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32, 16); > > + XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64, 16); > > + XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32, 16); > > + XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64, 16); > > XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32, 12); > > XFS_CHECK_STRUCT_SIZE(struct xfs_extent_64, 16); > > XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode, 176); > > @@ -146,6 +146,9 @@ xfs_check_ondisk_structs(void) > > XFS_CHECK_OFFSET(struct xfs_bui_log_format, > > bui_extents, 16); > > XFS_CHECK_OFFSET(struct xfs_cui_log_format, > > cui_extents, 16); > > XFS_CHECK_OFFSET(struct xfs_rui_log_format, > > rui_extents, 16); > > + XFS_CHECK_OFFSET(struct xfs_efi_log_format, > > efi_extents, 16); > > + XFS_CHECK_OFFSET(struct xfs_efi_log_format_32, > > efi_extents, 16); > > + XFS_CHECK_OFFSET(struct xfs_efi_log_format_64, > > efi_extents, 16); > > > > /* > > * The v5 superblock format extended several v4 header > > structures with > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > index f029c6702dda..8485e3b37ca0 100644 > > --- a/fs/xfs/xfs_super.c > > +++ b/fs/xfs/xfs_super.c > > @@ -2029,7 +2029,7 @@ xfs_init_caches(void) > > > > xfs_efd_cache = kmem_cache_create("xfs_efd_item", > > (sizeof(struct > > xfs_efd_log_item) + > > - (XFS_EFD_MAX_FAST_EXTENTS - > > 1) * > > + XFS_EFD_MAX_FAST_EXTENTS * > > sizeof(struct xfs_extent)), > > 0, 0, NULL); > > if (!xfs_efd_cache) > > @@ -2037,7 +2037,7 @@ xfs_init_caches(void) > > > > xfs_efi_cache = kmem_cache_create("xfs_efi_item", > > (sizeof(struct > > xfs_efi_log_item) + > > - (XFS_EFI_MAX_FAST_EXTENTS - > > 1) * > > + XFS_EFI_MAX_FAST_EXTENTS * > > sizeof(struct xfs_extent)), > > 0, 0, NULL); > > if (!xfs_efi_cache) > > >
On Mon, Oct 24, 2022 at 02:32:59PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of > memcpy. Since we're already fixing problems with BUI item copying, we > should fix it everything else. > > An extra difficulty here is that the ef[id]_extents arrays are declared > as single-element arrays. This is not the convention for flex arrays in > the modern kernel, and it causes all manner of problems with static > checking tools, since they often cannot tell the difference between a > single element array and a flex array. > > So for starters, change those array[1] declarations to array[] > declarations to signal that they are proper flex arrays and adjust all > the "size-1" expressions to fit the new declaration style. > > Next, refactor the xfs_efi_copy_format function to handle the copying of > the head and the flex array members separately. While we're at it, fix > a minor validation deficiency in the recovery function. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> Yes, it could be broken up into smaller changes. Yes, this could all be reworked like Christoph previously proposed. Yes, there are more cleanups that could be done. Yes, there are other ways to get to the same point. No, I don't think it is worth the effort to do it differently. Reviewed-by: Dave Chinner <dchinner@redhat.com>
diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h index b351b9dc6561..2f41fa8477c9 100644 --- a/fs/xfs/libxfs/xfs_log_format.h +++ b/fs/xfs/libxfs/xfs_log_format.h @@ -613,7 +613,7 @@ typedef struct xfs_efi_log_format { uint16_t efi_size; /* size of this item */ uint32_t efi_nextents; /* # extents to free */ uint64_t efi_id; /* efi identifier */ - xfs_extent_t efi_extents[1]; /* array of extents to free */ + xfs_extent_t efi_extents[]; /* array of extents to free */ } xfs_efi_log_format_t; typedef struct xfs_efi_log_format_32 { @@ -621,7 +621,7 @@ typedef struct xfs_efi_log_format_32 { uint16_t efi_size; /* size of this item */ uint32_t efi_nextents; /* # extents to free */ uint64_t efi_id; /* efi identifier */ - xfs_extent_32_t efi_extents[1]; /* array of extents to free */ + xfs_extent_32_t efi_extents[]; /* array of extents to free */ } __attribute__((packed)) xfs_efi_log_format_32_t; typedef struct xfs_efi_log_format_64 { @@ -629,7 +629,7 @@ typedef struct xfs_efi_log_format_64 { uint16_t efi_size; /* size of this item */ uint32_t efi_nextents; /* # extents to free */ uint64_t efi_id; /* efi identifier */ - xfs_extent_64_t efi_extents[1]; /* array of extents to free */ + xfs_extent_64_t efi_extents[]; /* array of extents to free */ } xfs_efi_log_format_64_t; /* @@ -642,7 +642,7 @@ typedef struct xfs_efd_log_format { uint16_t efd_size; /* size of this item */ uint32_t efd_nextents; /* # of extents freed */ uint64_t efd_efi_id; /* id of corresponding efi */ - xfs_extent_t efd_extents[1]; /* array of extents freed */ + xfs_extent_t efd_extents[]; /* array of extents freed */ } xfs_efd_log_format_t; typedef struct xfs_efd_log_format_32 { @@ -650,7 +650,7 @@ typedef struct xfs_efd_log_format_32 { uint16_t efd_size; /* size of this item */ uint32_t efd_nextents; /* # of extents freed */ uint64_t efd_efi_id; /* id of corresponding efi */ - xfs_extent_32_t efd_extents[1]; /* array of extents freed */ + xfs_extent_32_t efd_extents[]; /* array of extents freed */ } __attribute__((packed)) xfs_efd_log_format_32_t; typedef struct xfs_efd_log_format_64 { @@ -658,7 +658,7 @@ typedef struct xfs_efd_log_format_64 { uint16_t efd_size; /* size of this item */ uint32_t efd_nextents; /* # of extents freed */ uint64_t efd_efi_id; /* id of corresponding efi */ - xfs_extent_64_t efd_extents[1]; /* array of extents freed */ + xfs_extent_64_t efd_extents[]; /* array of extents freed */ } xfs_efd_log_format_64_t; /* diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c index 27ccfcd82f04..466cc5c5cd33 100644 --- a/fs/xfs/xfs_extfree_item.c +++ b/fs/xfs/xfs_extfree_item.c @@ -76,7 +76,7 @@ xfs_efi_item_sizeof( struct xfs_efi_log_item *efip) { return sizeof(struct xfs_efi_log_format) + - (efip->efi_format.efi_nextents - 1) * sizeof(xfs_extent_t); + efip->efi_format.efi_nextents * sizeof(xfs_extent_t); } STATIC void @@ -160,7 +160,7 @@ xfs_efi_init( ASSERT(nextents > 0); if (nextents > XFS_EFI_MAX_FAST_EXTENTS) { size = (uint)(sizeof(struct xfs_efi_log_item) + - ((nextents - 1) * sizeof(xfs_extent_t))); + (nextents * sizeof(xfs_extent_t))); efip = kmem_zalloc(size, 0); } else { efip = kmem_cache_zalloc(xfs_efi_cache, @@ -189,14 +189,19 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf, xfs_efi_log_format_t *dst_efi_fmt) xfs_efi_log_format_t *src_efi_fmt = buf->i_addr; uint i; uint len = sizeof(xfs_efi_log_format_t) + - (src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_t); + src_efi_fmt->efi_nextents * sizeof(xfs_extent_t); uint len32 = sizeof(xfs_efi_log_format_32_t) + - (src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_32_t); + src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t); uint len64 = sizeof(xfs_efi_log_format_64_t) + - (src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_64_t); + src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t); if (buf->i_len == len) { - memcpy((char *)dst_efi_fmt, (char*)src_efi_fmt, len); + memcpy(dst_efi_fmt, src_efi_fmt, + offsetof(struct xfs_efi_log_format, efi_extents)); + for (i = 0; i < src_efi_fmt->efi_nextents; i++) + memcpy(&dst_efi_fmt->efi_extents[i], + &src_efi_fmt->efi_extents[i], + sizeof(struct xfs_extent)); return 0; } else if (buf->i_len == len32) { xfs_efi_log_format_32_t *src_efi_fmt_32 = buf->i_addr; @@ -256,7 +261,7 @@ xfs_efd_item_sizeof( struct xfs_efd_log_item *efdp) { return sizeof(xfs_efd_log_format_t) + - (efdp->efd_format.efd_nextents - 1) * sizeof(xfs_extent_t); + efdp->efd_format.efd_nextents * sizeof(xfs_extent_t); } STATIC void @@ -341,7 +346,7 @@ xfs_trans_get_efd( if (nextents > XFS_EFD_MAX_FAST_EXTENTS) { efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) + - (nextents - 1) * sizeof(struct xfs_extent), + nextents * sizeof(struct xfs_extent), 0); } else { efdp = kmem_cache_zalloc(xfs_efd_cache, @@ -733,6 +738,12 @@ xlog_recover_efi_commit_pass2( efi_formatp = item->ri_buf[0].i_addr; + if (item->ri_buf[0].i_len < + offsetof(struct xfs_efi_log_format, efi_extents)) { + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp); + return -EFSCORRUPTED; + } + efip = xfs_efi_init(mp, efi_formatp->efi_nextents); error = xfs_efi_copy_format(&item->ri_buf[0], &efip->efi_format); if (error) { @@ -772,9 +783,9 @@ xlog_recover_efd_commit_pass2( efd_formatp = item->ri_buf[0].i_addr; ASSERT((item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_32_t) + - ((efd_formatp->efd_nextents - 1) * sizeof(xfs_extent_32_t)))) || + (efd_formatp->efd_nextents * sizeof(xfs_extent_32_t)))) || (item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_64_t) + - ((efd_formatp->efd_nextents - 1) * sizeof(xfs_extent_64_t))))); + (efd_formatp->efd_nextents * sizeof(xfs_extent_64_t))))); xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp->efd_efi_id); return 0; diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h index 19c1df00b48e..9737b5a9f405 100644 --- a/fs/xfs/xfs_ondisk.h +++ b/fs/xfs/xfs_ondisk.h @@ -118,10 +118,10 @@ xfs_check_ondisk_structs(void) /* log structures */ XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format, 88); XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat, 24); - XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32, 28); - XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64, 32); - XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32, 28); - XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64, 32); + XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32, 16); + XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64, 16); + XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32, 16); + XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64, 16); XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32, 12); XFS_CHECK_STRUCT_SIZE(struct xfs_extent_64, 16); XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode, 176); @@ -146,6 +146,9 @@ xfs_check_ondisk_structs(void) XFS_CHECK_OFFSET(struct xfs_bui_log_format, bui_extents, 16); XFS_CHECK_OFFSET(struct xfs_cui_log_format, cui_extents, 16); XFS_CHECK_OFFSET(struct xfs_rui_log_format, rui_extents, 16); + XFS_CHECK_OFFSET(struct xfs_efi_log_format, efi_extents, 16); + XFS_CHECK_OFFSET(struct xfs_efi_log_format_32, efi_extents, 16); + XFS_CHECK_OFFSET(struct xfs_efi_log_format_64, efi_extents, 16); /* * The v5 superblock format extended several v4 header structures with diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index f029c6702dda..8485e3b37ca0 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -2029,7 +2029,7 @@ xfs_init_caches(void) xfs_efd_cache = kmem_cache_create("xfs_efd_item", (sizeof(struct xfs_efd_log_item) + - (XFS_EFD_MAX_FAST_EXTENTS - 1) * + XFS_EFD_MAX_FAST_EXTENTS * sizeof(struct xfs_extent)), 0, 0, NULL); if (!xfs_efd_cache) @@ -2037,7 +2037,7 @@ xfs_init_caches(void) xfs_efi_cache = kmem_cache_create("xfs_efi_item", (sizeof(struct xfs_efi_log_item) + - (XFS_EFI_MAX_FAST_EXTENTS - 1) * + XFS_EFI_MAX_FAST_EXTENTS * sizeof(struct xfs_extent)), 0, 0, NULL); if (!xfs_efi_cache)