From: Allison Henderson <allison.henderson@oracle.com>
To: "djwong@kernel.org" <djwong@kernel.org>
Cc: "david@fromorbit.com" <david@fromorbit.com>,
"linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH 5/6] xfs: fix memcpy fortify errors in EFI log format copying
Date: Tue, 25 Oct 2022 20:54:25 +0000 [thread overview]
Message-ID: <fca71fe8808ba11e6e96cc5cc4c2da3acb243a7d.camel@oracle.com> (raw)
In-Reply-To: <166664717980.2688790.14877643421674738495.stgit@magnolia>
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)
>
next prev parent reply other threads:[~2022-10-25 20:54 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
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 [this message]
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=fca71fe8808ba11e6e96cc5cc4c2da3acb243a7d.camel@oracle.com \
--to=allison.henderson@oracle.com \
--cc=david@fromorbit.com \
--cc=djwong@kernel.org \
--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