lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y1cFxnea750izJd7@magnolia> Date: Mon, 24 Oct 2022 14:38:14 -0700 From: "Darrick J. Wong" <djwong@...nel.org> To: Kees Cook <keescook@...omium.org>, h@...nolia Cc: xfs <linux-xfs@...r.kernel.org>, Zorro Lang <zlang@...hat.com>, linux-hardening@...r.kernel.org Subject: Re: [RFC PATCH] xfs: fix FORTIFY_SOURCE complaints about log item memcpy On Mon, Oct 24, 2022 at 09:59:08AM -0700, Kees Cook wrote: > On Wed, Oct 19, 2022 at 05:04:11PM -0700, Darrick J. Wong wrote: > > [...] > > -/* > > - * Copy an BUI format buffer from the given buf, and into the destination > > - * BUI format structure. The BUI/BUD items were designed not to need any > > - * special alignment handling. > > - */ > > -static int > > -xfs_bui_copy_format( > > - struct xfs_log_iovec *buf, > > - struct xfs_bui_log_format *dst_bui_fmt) > > -{ > > - struct xfs_bui_log_format *src_bui_fmt; > > - uint len; > > - > > - src_bui_fmt = buf->i_addr; > > - len = xfs_bui_log_format_sizeof(src_bui_fmt->bui_nextents); > > - > > - if (buf->i_len == len) { > > - memcpy(dst_bui_fmt, src_bui_fmt, len); > > - return 0; > > - } > > - XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL); > > - return -EFSCORRUPTED; > > -} > > This is the place where flex_cpy() could be used: > > flex_cpy(dst_bui_fmt, src_bui_fmt); <nod> > > [...] > > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c > > index 51f66e982484..5367e404aa0f 100644 > > --- a/fs/xfs/xfs_bmap_item.c > > +++ b/fs/xfs/xfs_bmap_item.c > > @@ -590,7 +590,7 @@ xfs_bui_item_relog( > > set_bit(XFS_LI_DIRTY, &budp->bud_item.li_flags); > > > > buip = xfs_bui_init(tp->t_mountp); > > - memcpy(buip->bui_format.bui_extents, extp, count * sizeof(*extp)); > > + memcpy_array(buip->bui_format.bui_extents, extp, count, sizeof(*extp)); > > atomic_set(&buip->bui_next_extent, count); > > xfs_trans_add_item(tp, &buip->bui_item); > > set_bit(XFS_LI_DIRTY, &buip->bui_item.li_flags); > > Looking more closely, I don't understand why this is treated as a flex > array when it's actually fixed size: > > xfs_bui_init(): > buip = kmem_cache_zalloc(xfs_bui_cache, GFP_KERNEL | __GFP_NOFAIL); > ... > buip->bui_format.bui_nextents = XFS_BUI_MAX_FAST_EXTENTS; > > fs/xfs/xfs_bmap_item.h:#define XFS_BUI_MAX_FAST_EXTENTS 1 Yeah, after a few more iterations of this patchset I realized that *most* of the _relog functions are fine, it's only the one for EFI items that trips over the not-flex array[1] definition. I decided that the proper fix for that was simply to fix the field definition to follow the modern form for flex arrays. > > [...] > > +/* > > + * Copy an array from @src into the @dst buffer, allowing for @dst to be a > > + * structure with a VLAs at the end. gcc11 is smart enough for > > + * __builtin_object_size to see through void * arguments to static inline > > + * function but not to detect VLAs, which leads to kernel warnings. > > + */ > > +static inline int memcpy_array(void *dst, void *src, size_t nmemb, size_t size) > > +{ > > + size_t bytes; > > + > > + if (unlikely(check_mul_overflow(nmemb, size, &bytes))) { > > + ASSERT(0); > > + return -ENOMEM; > > + } > > + > > + unsafe_memcpy(dst, src, bytes, VLA size detection broken on gcc11 ); > > + return 0; > > +} > > This "unsafe_memcpy" isn't needed. FORTIFY won't warn on this copy: > the destination is a flex array member, not a flex array struct > (i.e. __builtin_object_size() here will report "-1", rather than a > fixed size). And while the type bounds checking for overflow is nice, > it should also be checking the allocated size. (i.e. how large is "dst"? > this helper only knows how large src is.) <nod> I realized that these helpers introducing unsafe memcpy weren't needed. Later on after chatting with dchinner a bit I came to the conclusion that we might as well convert most of the _copy_format functions to memcpy the structure head and flex array separately since that function is converting an ondisk log item into its in-memory representation, and some day we'll make those struct fields endian safe. They aren't now, and that's one of the (many) gaping holes that need fixing. I sent my candidate fixes series to the list just now. --D > > -Kees > > -- > Kees Cook
Powered by blists - more mailing lists