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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.