On Wed, Oct 19, 2022 at 05:04:11PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > Starting in 6.1, CONFIG_FORTIFY_SOURCE checks the length parameter of > memcpy. Unfortunately, it doesn't handle VLAs correctly: Nit-pick on terminology: these are "flexible array structures" (structures that end with a "flexible array member"); VLAs are a different (removed from the kernel) beast. > memcpy: detected field-spanning write (size 48) of single field "dst_bui_fmt" at fs/xfs/xfs_bmap_item.c:628 (size 16) Step right up; XFS is next to trip[1] this check. Let's get this fixed... > We know the memcpy going > on here is correct because I've run all the log recovery tests with > KASAN turned on, and it does not detect actual memory misuse. Yup, this is a false positive. > My first attempt to work around this problem was to cast the arguments > [...] > My second attempt changed the cast to a (void *), with the same results > [...] > My third attempt was to pass the void pointers directly into > [...] > My fourth attempt collapsed the _copy_format function into the callers > [...] The point here is to use a better API, which is fallible and has the ability to perform the bounds checking itself. I had proposed an initial version of this idea here[2]. [1] https://lore.kernel.org/all/?q=%22field-spanning+write%22 [2] https://lore.kernel.org/llvm/20220504014440.3697851-3-keescook@xxxxxxxxxxxx/ > "These cases end up appearing to the compiler to be sized as if the > flexible array had 0 elements. :( For more details see: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832 > https://godbolt.org/z/vW6x8vh4P ". > > I don't /quite/ think that turning off CONFIG_FORTIFY_SOURCE is the > right solution here, but in the meantime this is causing a lot of fstest > failures, and I really need to get back to fixing user reported data > corruption problems instead of dealing with gcc stupidity. :( I think XFS could be a great first candidate for using something close to the proposed flex_cpy() API. What do you think of replacing the memcpy() calls with something like this instead: - if (buf->i_len == len) { - memcpy(dst_bui_fmt, src_bui_fmt, len); - return 0; - } + if (buf->i_len == len && + flex_cpy(dst_bui_fmt, src_bui_fmt, + bui_nextents, bui_extents) == 0) return 0; XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL); return -EFSCORRUPTED; To avoid passing in the element count and element array fields, the alias macros could be used: struct xfs_bui_log_format { uint16_t bui_type; /* bui log item type */ uint16_t bui_size; /* size of this item */ /* # extents to free */ DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(uint32_t, bui_nextents); uint64_t bui_id; /* bui identifier */ /* array of extents to bmap */ DECLARE_FLEX_ARRAY_ELEMENTS(struct xfs_map_extent, bui_extents); }; What do you think about these options? In the meantime, unsafe_memcpy() should be fine for v6.1. BTW, this FORTIFY_SOURCE change was present in linux-next for the entire prior development cycle. Are the xfstests not run on -next kernels? -Kees -- Kees Cook
