| Message ID | 20220504014440.3697851-1-keescook@chromium.org |
|---|---|
| Headers | show |
| Series | Introduce flexible array struct memcpy() helpers | expand |
On Wed, May 4, 2022 at 9:47 AM Kees Cook <keescook@chromium.org> wrote: > > Add tests for the new flexible array structure helpers. These can be run > with: > > make ARCH=um mrproper > ./tools/testing/kunit/kunit.py config Nit: it shouldn't be necessary to run kunit.py config separately: kunit.py run will configure the kernel if necessary. > ./tools/testing/kunit/kunit.py run flex_array > > Cc: David Gow <davidgow@google.com> > Cc: kunit-dev@googlegroups.com > Signed-off-by: Kees Cook <keescook@chromium.org> > --- This looks pretty good to me: it certainly worked on the different setups I tried (um, x86_64, x86_64+KASAN). A few minor nitpicks inline, mostly around minor config-y things, or things which weren't totally clear on my first read-through. Hopefully one day, with the various stubbing features or something similar, we'll be able to check against allocation failures in flex_dup(), too, but otherwise nothing seems too obviously missing. Reviewed-by: David Gow <davidgow@google.com> -- David > lib/Kconfig.debug | 12 +- > lib/Makefile | 1 + > lib/flex_array_kunit.c | 523 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 531 insertions(+), 5 deletions(-) > create mode 100644 lib/flex_array_kunit.c > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 9077bb38bc93..8bae6b169c50 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -2551,11 +2551,6 @@ config OVERFLOW_KUNIT_TEST > Builds unit tests for the check_*_overflow(), size_*(), allocation, and > related functions. > > - For more information on KUnit and unit tests in general please refer > - to the KUnit documentation in Documentation/dev-tools/kunit/. > - > - If unsure, say N. > - Nit: while I'm not against removing some of this boilerplate, is it better suited for a separate commit? > config STACKINIT_KUNIT_TEST > tristate "Test level of stack variable initialization" if !KUNIT_ALL_TESTS > depends on KUNIT > @@ -2567,6 +2562,13 @@ config STACKINIT_KUNIT_TEST > CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF, > or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. > > +config FLEX_ARRAY_KUNIT_TEST > + tristate "Test flex_*() family of helper functions at runtime" if !KUNIT_ALL_TESTS > + depends on KUNIT > + default KUNIT_ALL_TESTS > + help > + Builds unit tests for flexible array copy helper functions. > + Nit: checkpatch warns that the description here may be insufficient: WARNING: please write a help paragraph that fully describes the config symbol > config TEST_UDELAY > tristate "udelay test driver" > help > diff --git a/lib/Makefile b/lib/Makefile > index 6b9ffc1bd1ee..9884318db330 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -366,6 +366,7 @@ obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o > obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o > CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable) > obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o > +obj-$(CONFIG_FLEX_ARRAY_KUNIT_TEST) += flex_array_kunit.o > > obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o > > diff --git a/lib/flex_array_kunit.c b/lib/flex_array_kunit.c > new file mode 100644 > index 000000000000..48bee88945b4 > --- /dev/null > +++ b/lib/flex_array_kunit.c > @@ -0,0 +1,523 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Test cases for flex_*() array manipulation helpers. > + */ > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <kunit/test.h> > +#include <linux/device.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/flex_array.h> > + > +#define COMPARE_STRUCTS(STRUCT_A, STRUCT_B) do { \ > + STRUCT_A *ptr_A; \ > + STRUCT_B *ptr_B; \ > + int rc; \ > + size_t size_A, size_B; \ > + \ > + /* matching types for flex array elements and count */ \ > + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A), sizeof(*ptr_B)); \ > + KUNIT_EXPECT_TRUE(test, __same_type(*ptr_A->data, \ > + *ptr_B->__flex_array_elements)); \ > + KUNIT_EXPECT_TRUE(test, __same_type(ptr_A->datalen, \ > + ptr_B->__flex_array_elements_count)); \ > + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A->data), \ > + sizeof(*ptr_B->__flex_array_elements)); \ > + KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), data), \ > + offsetof(typeof(*ptr_B), \ > + __flex_array_elements)); \ > + KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), datalen), \ > + offsetof(typeof(*ptr_B), \ > + __flex_array_elements_count)); \ > + \ > + /* struct_size() vs __fas_bytes() */ \ > + size_A = struct_size(ptr_A, data, 13); \ > + rc = __fas_bytes(ptr_B, __flex_array_elements, \ > + __flex_array_elements_count, 13, &size_B); \ > + KUNIT_EXPECT_EQ(test, rc, 0); \ > + KUNIT_EXPECT_EQ(test, size_A, size_B); \ > + \ > + /* flex_array_size() vs __fas_elements_bytes() */ \ > + size_A = flex_array_size(ptr_A, data, 13); \ > + rc = __fas_elements_bytes(ptr_B, __flex_array_elements, \ > + __flex_array_elements_count, 13, &size_B); \ > + KUNIT_EXPECT_EQ(test, rc, 0); \ > + KUNIT_EXPECT_EQ(test, size_A, size_B); \ > + \ > + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A) + size_A, \ > + offsetof(typeof(*ptr_A), data) + \ > + (sizeof(*ptr_A->data) * 13)); \ > + KUNIT_EXPECT_EQ(test, sizeof(*ptr_B) + size_B, \ > + offsetof(typeof(*ptr_B), \ > + __flex_array_elements) + \ > + (sizeof(*ptr_B->__flex_array_elements) * \ > + 13)); \ > +} while (0) > + > +struct normal { > + size_t datalen; > + u32 data[]; > +}; > + > +struct decl_normal { > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, datalen); > + DECLARE_FLEX_ARRAY_ELEMENTS(u32, data); > +}; > + > +struct aligned { > + unsigned short datalen; > + char data[] __aligned(__alignof__(u64)); > +}; > + > +struct decl_aligned { > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(unsigned short, datalen); > + DECLARE_FLEX_ARRAY_ELEMENTS(char, data) __aligned(__alignof__(u64)); > +}; > + > +static void struct_test(struct kunit *test) > +{ > + COMPARE_STRUCTS(struct normal, struct decl_normal); > + COMPARE_STRUCTS(struct aligned, struct decl_aligned); > +} If I understand it, the purpose of this is to ensure that structs both with and without the flexible array declaration have the same memory layout? If so, any chance of a comment briefly stating that's the purpose (or renaming this test struct_layout_test())? Also, would it make sense to do the same with the struct with internal padding below? > + > +/* Flexible array structure with internal padding. */ > +struct flex_cpy_obj { > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u8, count); > + unsigned long empty; > + char induce_padding; > + /* padding ends up here */ > + unsigned long after_padding; > + DECLARE_FLEX_ARRAY_ELEMENTS(u32, flex); > +}; > + > +/* Encapsulating flexible array structure. */ > +struct flex_dup_obj { > + unsigned long flags; > + int junk; > + struct flex_cpy_obj fas; > +}; > + > +/* Flexible array struct of only bytes. */ > +struct tiny_flex { > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u8, count); > + DECLARE_FLEX_ARRAY_ELEMENTS(u8, byte_array); > +}; > + > +#define CHECK_COPY(ptr) do { \ > + typeof(*(ptr)) *_cc_dst = (ptr); \ > + KUNIT_EXPECT_EQ(test, _cc_dst->induce_padding, 0); \ > + memcpy(&padding, &_cc_dst->induce_padding + sizeof(_cc_dst->induce_padding), \ > + sizeof(padding)); \ > + /* Padding should be zero too. */ \ > + KUNIT_EXPECT_EQ(test, padding, 0); \ > + KUNIT_EXPECT_EQ(test, src->count, _cc_dst->count); \ > + KUNIT_EXPECT_EQ(test, _cc_dst->count, TEST_TARGET); \ > + for (i = 0; i < _cc_dst->count - 1; i++) { \ > + /* 'A' is 0x41, and here repeated in a u32. */ \ Would it be simpler to just note that the magic value is 0x41, rather than have it be the character 'A'? > + KUNIT_EXPECT_EQ(test, _cc_dst->flex[i], 0x41414141); \ > + } \ > + /* Last item should be different. */ \ > + KUNIT_EXPECT_EQ(test, _cc_dst->flex[_cc_dst->count - 1], 0x14141414); \ > +} while (0) > + > +/* Test copying from one flexible array struct into another. */ > +static void flex_cpy_test(struct kunit *test) > +{ > +#define TEST_BOUNDS 13 > +#define TEST_TARGET 12 > +#define TEST_SMALL 10 > + struct flex_cpy_obj *src, *dst; > + unsigned long padding; > + int i, rc; > + > + /* Prepare open-coded source. */ > + src = kzalloc(struct_size(src, flex, TEST_BOUNDS), GFP_KERNEL); > + src->count = TEST_BOUNDS; > + memset(src->flex, 'A', flex_array_size(src, flex, TEST_BOUNDS)); As above, it's possibly nicer to just state 0x41 here, rather than 'A', since all we're doing is checking against a hex value. > + src->flex[src->count - 2] = 0x14141414; > + src->flex[src->count - 1] = 0x24242424; > + > + /* Prepare open-coded destination, alloc only. */ > + dst = kzalloc(struct_size(src, flex, TEST_BOUNDS), GFP_KERNEL); > + /* Pre-fill with 0xFE marker. */ > + memset(dst, 0xFE, struct_size(src, flex, TEST_BOUNDS)); > + /* Pretend we're 1 element smaller. */ > + dst->count = TEST_TARGET; > + > + /* Pretend to match the target destination size. */ > + src->count = TEST_TARGET; > + > + rc = flex_cpy(dst, src); > + KUNIT_EXPECT_EQ(test, rc, 0); > + CHECK_COPY(dst); > + /* Item past last copied item is unchanged from initial memset. */ > + KUNIT_EXPECT_EQ(test, dst->flex[dst->count], 0xFEFEFEFE); > + > + /* Now trip overflow, and verify we didn't clobber beyond end. */ > + src->count = TEST_BOUNDS; > + rc = flex_cpy(dst, src); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + /* Item past last copied item is unchanged from initial memset. */ > + KUNIT_EXPECT_EQ(test, dst->flex[dst->count], 0xFEFEFEFE); > + > + /* Reset destination contents. */ > + memset(dst, 0xFD, struct_size(src, flex, TEST_BOUNDS)); > + dst->count = TEST_TARGET; > + > + /* Copy less than max. */ > + src->count = TEST_SMALL; > + rc = flex_cpy(dst, src); > + KUNIT_EXPECT_EQ(test, rc, 0); > + /* Verify count was adjusted. */ > + KUNIT_EXPECT_EQ(test, dst->count, TEST_SMALL); > + /* Verify element beyond src size was wiped. */ > + KUNIT_EXPECT_EQ(test, dst->flex[TEST_SMALL], 0); > + /* Verify element beyond original dst size was untouched. */ > + KUNIT_EXPECT_EQ(test, dst->flex[TEST_TARGET], 0xFDFDFDFD); > + > + kfree(dst); > + kfree(src); > +#undef TEST_BOUNDS > +#undef TEST_TARGET > +#undef TEST_SMALL > +} > + > +static void flex_dup_test(struct kunit *test) > +{ > +#define TEST_TARGET 12 > + struct flex_cpy_obj *src, *dst = NULL, **null = NULL; > + struct flex_dup_obj *encap = NULL; > + unsigned long padding; > + int i, rc; > + > + /* Prepare open-coded source. */ > + src = kzalloc(struct_size(src, flex, TEST_TARGET), GFP_KERNEL); > + src->count = TEST_TARGET; > + memset(src->flex, 'A', flex_array_size(src, flex, TEST_TARGET)); > + src->flex[src->count - 1] = 0x14141414; > + > + /* Reject NULL @alloc. */ > + rc = flex_dup(null, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + > + /* Check good copy. */ > + rc = flex_dup(&dst, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_ASSERT_TRUE(test, dst != NULL); > + CHECK_COPY(dst); > + > + /* Reject non-NULL *@alloc. */ > + rc = flex_dup(&dst, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + > + kfree(dst); > + > + /* Check good encap copy. */ > + rc = __flex_dup(&encap, .fas, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_ASSERT_TRUE(test, dst != NULL); > + CHECK_COPY(&encap->fas); > + /* Check that items external to "fas" are zero. */ > + KUNIT_EXPECT_EQ(test, encap->flags, 0); > + KUNIT_EXPECT_EQ(test, encap->junk, 0); > + kfree(encap); > +#undef MAGIC_WORD MAGIC_WORD isn't defined (or used) for flux_dup_test? Is it worth using it (or something similar) for the 'A' / 0x14141414 and the CHECK_COPY() macro? > +#undef TEST_TARGET > +} > + > +static void mem_to_flex_test(struct kunit *test) > +{ > +#define TEST_TARGET 9 > +#define TEST_MAX U8_MAX > +#define MAGIC_WORD 0x03030303 > + u8 magic_byte = MAGIC_WORD & 0xff; > + struct flex_cpy_obj *dst; > + size_t big = (size_t)INT_MAX + 1; > + char small[] = "Hello"; > + char *src; > + u32 src_len; > + int rc; > + > + /* Open coded allocations, 1 larger than actually used. */ > + src_len = flex_array_size(dst, flex, TEST_MAX + 1); > + src = kzalloc(src_len, GFP_KERNEL); > + dst = kzalloc(struct_size(dst, flex, TEST_MAX + 1), GFP_KERNEL); > + dst->count = TEST_TARGET; > + > + /* Fill source. */ > + memset(src, magic_byte, src_len); > + > + /* Short copy is fine. */ > + KUNIT_EXPECT_EQ(test, dst->flex[0], 0); > + KUNIT_EXPECT_EQ(test, dst->flex[1], 0); > + rc = mem_to_flex(dst, src, 1); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_EXPECT_EQ(test, dst->count, 1); > + KUNIT_EXPECT_EQ(test, dst->after_padding, 0); > + KUNIT_EXPECT_EQ(test, dst->flex[0], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, dst->flex[1], 0); > + dst->count = TEST_TARGET; > + > + /* Reject negative elements count. */ > + rc = mem_to_flex(dst, small, -1); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + /* Make sure dst is unchanged. */ > + KUNIT_EXPECT_EQ(test, dst->flex[0], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, dst->flex[1], 0); > + > + /* Reject compile-time read overflow. */ > + rc = mem_to_flex(dst, small, 20); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + /* Make sure dst is unchanged. */ > + KUNIT_EXPECT_EQ(test, dst->flex[0], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, dst->flex[1], 0); > + > + /* Reject giant buffer source. */ > + rc = mem_to_flex(dst, small, big); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + /* Make sure dst is unchanged. */ > + KUNIT_EXPECT_EQ(test, dst->flex[0], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, dst->flex[1], 0); > + > + /* Copy beyond storage size is rejected. */ > + dst->count = TEST_MAX; > + KUNIT_EXPECT_EQ(test, dst->flex[TEST_MAX - 1], 0); > + KUNIT_EXPECT_EQ(test, dst->flex[TEST_MAX], 0); > + rc = mem_to_flex(dst, src, TEST_MAX + 1); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + /* Make sure dst is unchanged. */ > + KUNIT_EXPECT_EQ(test, dst->flex[0], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, dst->flex[1], 0); > + > + kfree(dst); > + kfree(src); > +#undef MAGIC_WORD > +#undef TEST_MAX > +#undef TEST_TARGET > +} > + > +static void mem_to_flex_dup_test(struct kunit *test) > +{ > +#define ELEMENTS_COUNT 259 > +#define MAGIC_WORD 0xABABABAB > + u8 magic_byte = MAGIC_WORD & 0xff; > + struct flex_dup_obj *obj = NULL; > + struct tiny_flex *tiny = NULL, **null = NULL; > + size_t src_len, count, big = (size_t)INT_MAX + 1; > + char small[] = "Hello"; > + u8 *src; > + int rc; > + > + src_len = struct_size(tiny, byte_array, ELEMENTS_COUNT); > + src = kzalloc(src_len, GFP_KERNEL); > + KUNIT_ASSERT_TRUE(test, src != NULL); > + /* Fill with bytes. */ > + memset(src, magic_byte, src_len); > + KUNIT_EXPECT_EQ(test, src[0], magic_byte); > + KUNIT_EXPECT_EQ(test, src[src_len / 2], magic_byte); > + KUNIT_EXPECT_EQ(test, src[src_len - 1], magic_byte); > + > + /* Reject storage exceeding elements_count type. */ > + count = ELEMENTS_COUNT; > + rc = mem_to_flex_dup(&tiny, src, count, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + KUNIT_EXPECT_TRUE(test, tiny == NULL); > + > + /* Reject negative elements count. */ > + rc = mem_to_flex_dup(&tiny, src, -1, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + KUNIT_EXPECT_TRUE(test, tiny == NULL); > + > + /* Reject compile-time read overflow. */ > + rc = mem_to_flex_dup(&tiny, small, 20, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + KUNIT_EXPECT_TRUE(test, tiny == NULL); > + > + /* Reject giant buffer source. */ > + rc = mem_to_flex_dup(&tiny, small, big, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + KUNIT_EXPECT_TRUE(test, tiny == NULL); > + > + /* Reject NULL @alloc. */ > + rc = mem_to_flex_dup(null, src, count, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + > + /* Allow reasonable count.*/ > + count = ELEMENTS_COUNT / 2; > + rc = mem_to_flex_dup(&tiny, src, count, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_ASSERT_TRUE(test, tiny != NULL); > + /* Spot check the copy happened. */ > + KUNIT_EXPECT_EQ(test, tiny->count, count); > + KUNIT_EXPECT_EQ(test, tiny->byte_array[0], magic_byte); > + KUNIT_EXPECT_EQ(test, tiny->byte_array[count / 2], magic_byte); > + KUNIT_EXPECT_EQ(test, tiny->byte_array[count - 1], magic_byte); > + > + /* Reject non-NULL *@alloc. */ > + rc = mem_to_flex_dup(&tiny, src, count, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + kfree(tiny); > + > + /* Works with encapsulation too. */ > + count = ELEMENTS_COUNT / 10; > + rc = __mem_to_flex_dup(&obj, .fas, src, count, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_ASSERT_TRUE(test, obj != NULL); > + /* Spot check the copy happened. */ > + KUNIT_EXPECT_EQ(test, obj->fas.count, count); > + KUNIT_EXPECT_EQ(test, obj->fas.after_padding, 0); > + KUNIT_EXPECT_EQ(test, obj->fas.flex[0], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, obj->fas.flex[count / 2], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, obj->fas.flex[count - 1], MAGIC_WORD); > + /* Check members before flexible array struct are zero. */ > + KUNIT_EXPECT_EQ(test, obj->flags, 0); > + KUNIT_EXPECT_EQ(test, obj->junk, 0); > + kfree(obj); > +#undef MAGIC_WORD > +#undef ELEMENTS_COUNT > +} > + > +static void flex_to_mem_test(struct kunit *test) > +{ > +#define ELEMENTS_COUNT 200 > +#define MAGIC_WORD 0xF1F2F3F4 > + struct flex_cpy_obj *src; > + typeof(*src->flex) *cast; > + size_t src_len = struct_size(src, flex, ELEMENTS_COUNT); > + size_t copy_len = flex_array_size(src, flex, ELEMENTS_COUNT); > + int i, rc; > + size_t bytes = 0; > + u8 too_small; > + u8 *dst; > + > + /* Create a filled flexible array struct. */ > + src = kzalloc(src_len, GFP_KERNEL); > + KUNIT_ASSERT_TRUE(test, src != NULL); > + src->count = ELEMENTS_COUNT; > + src->after_padding = 13; > + for (i = 0; i < ELEMENTS_COUNT; i++) > + src->flex[i] = MAGIC_WORD; > + > + /* Over-allocate space to do past-src_len checking. */ > + dst = kzalloc(src_len * 2, GFP_KERNEL); > + KUNIT_ASSERT_TRUE(test, dst != NULL); > + cast = (void *)dst; > + > + /* Fail if dst is too small. */ > + rc = flex_to_mem(dst, copy_len - 1, src, &bytes); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + /* Make sure nothing was copied. */ > + KUNIT_EXPECT_EQ(test, bytes, 0); > + KUNIT_EXPECT_EQ(test, cast[0], 0); > + > + /* Fail if type too small to hold size of copy. */ > + KUNIT_EXPECT_GT(test, copy_len, type_max(typeof(too_small))); > + rc = flex_to_mem(dst, copy_len, src, &too_small); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + /* Make sure nothing was copied. */ > + KUNIT_EXPECT_EQ(test, bytes, 0); > + KUNIT_EXPECT_EQ(test, cast[0], 0); > + > + /* Check good copy. */ > + rc = flex_to_mem(dst, copy_len, src, &bytes); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_EXPECT_EQ(test, bytes, copy_len); > + /* Spot check the copy */ > + KUNIT_EXPECT_EQ(test, cast[0], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, cast[ELEMENTS_COUNT / 2], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, cast[ELEMENTS_COUNT - 1], MAGIC_WORD); > + /* Make sure nothing was written after last element. */ > + KUNIT_EXPECT_EQ(test, cast[ELEMENTS_COUNT], 0); > + > + kfree(dst); > + kfree(src); > +#undef MAGIC_WORD > +#undef ELEMENTS_COUNT > +} > + > +static void flex_to_mem_dup_test(struct kunit *test) > +{ > +#define ELEMENTS_COUNT 210 > +#define MAGIC_WORD 0xF0F1F2F3 > + struct flex_dup_obj *obj, **null = NULL; > + struct flex_cpy_obj *src; > + typeof(*src->flex) *cast; > + size_t obj_len = struct_size(obj, fas.flex, ELEMENTS_COUNT); > + size_t src_len = struct_size(src, flex, ELEMENTS_COUNT); > + size_t copy_len = flex_array_size(src, flex, ELEMENTS_COUNT); > + int i, rc; > + size_t bytes = 0; > + u8 too_small = 0; > + u8 *dst = NULL; > + > + /* Create a filled flexible array struct. */ > + obj = kzalloc(obj_len, GFP_KERNEL); > + KUNIT_ASSERT_TRUE(test, obj != NULL); > + obj->fas.count = ELEMENTS_COUNT; > + obj->fas.after_padding = 13; > + for (i = 0; i < ELEMENTS_COUNT; i++) > + obj->fas.flex[i] = MAGIC_WORD; > + src = &obj->fas; > + > + /* Fail if type too small to hold size of copy. */ > + KUNIT_EXPECT_GT(test, src_len, type_max(typeof(too_small))); > + rc = flex_to_mem_dup(&dst, &too_small, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + KUNIT_EXPECT_TRUE(test, dst == NULL); > + KUNIT_EXPECT_EQ(test, too_small, 0); > + > + /* Fail if @alloc_size is NULL. */ > + KUNIT_EXPECT_TRUE(test, dst == NULL); > + rc = flex_to_mem_dup(&dst, dst, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + KUNIT_EXPECT_TRUE(test, dst == NULL); > + > + /* Fail if @alloc is NULL. */ > + rc = flex_to_mem_dup(null, &bytes, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + KUNIT_EXPECT_TRUE(test, dst == NULL); > + KUNIT_EXPECT_EQ(test, bytes, 0); > + > + /* Check good copy. */ > + rc = flex_to_mem_dup(&dst, &bytes, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_EXPECT_TRUE(test, dst != NULL); > + KUNIT_EXPECT_EQ(test, bytes, copy_len); > + cast = (void *)dst; > + /* Spot check the copy */ > + KUNIT_EXPECT_EQ(test, cast[0], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, cast[ELEMENTS_COUNT / 2], MAGIC_WORD); > + KUNIT_EXPECT_EQ(test, cast[ELEMENTS_COUNT - 1], MAGIC_WORD); > + > + /* Fail if *@alloc is non-NULL. */ > + bytes = 0; > + rc = flex_to_mem_dup(&dst, &bytes, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + KUNIT_EXPECT_EQ(test, bytes, 0); > + > + kfree(dst); > + kfree(obj); > +#undef MAGIC_WORD > +#undef ELEMENTS_COUNT > +} > + > +static struct kunit_case flex_array_test_cases[] = { > + KUNIT_CASE(struct_test), > + KUNIT_CASE(flex_cpy_test), > + KUNIT_CASE(flex_dup_test), > + KUNIT_CASE(mem_to_flex_test), > + KUNIT_CASE(mem_to_flex_dup_test), > + KUNIT_CASE(flex_to_mem_test), > + KUNIT_CASE(flex_to_mem_dup_test), > + {} > +}; > + > +static struct kunit_suite flex_array_test_suite = { > + .name = "flex_array", > + .test_cases = flex_array_test_cases, > +}; > + > +kunit_test_suite(flex_array_test_suite); > + > +MODULE_LICENSE("GPL"); > -- > 2.32.0 >
On Tue, May 03, 2022 at 10:31:05PM -0500, Gustavo A. R. Silva wrote: > On Tue, May 03, 2022 at 06:44:10PM -0700, Kees Cook wrote: > [...] > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > > index 1b5a9c2e1c29..09346aee1022 100644 > > --- a/net/netlink/af_netlink.c > > +++ b/net/netlink/af_netlink.c > > @@ -2445,7 +2445,10 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err, > > NLMSG_ERROR, payload, flags); > > errmsg = nlmsg_data(rep); > > errmsg->error = err; > > - memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh)); > > + errmsg->msg = *nlh; > > + if (payload > sizeof(*errmsg)) > > + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, > > + nlh->nlmsg_len - sizeof(*nlh)); > > They have nlmsg_len()[1] for the length of the payload without the header: > > /** > * nlmsg_len - length of message payload > * @nlh: netlink message header > */ > static inline int nlmsg_len(const struct nlmsghdr *nlh) > { > return nlh->nlmsg_len - NLMSG_HDRLEN; > } Oh, hm, yeah, that would be much cleaner. The relationship between "payload" and nlmsg_len is confusing in here. :) So, this should be simpler: - memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh)); + errmsg->msg = *nlh; + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, nlmsg_len(nlh)); It's actually this case that triggered my investigation in __bos(1)'s misbehavior around sub-structs, since this case wasn't getting silenced: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832 It still feels like it should be possible to get this right without splitting the memcpy, though. Hmpf. > (would that function use some sanitization, though? what if nlmsg_len is > somehow manipulated to be less than NLMSG_HDRLEN?...) Maybe something like: static inline int nlmsg_len(const struct nlmsghdr *nlh) { if (WARN_ON(nlh->nlmsg_len < NLMSG_HDRLEN)) return 0; return nlh->nlmsg_len - NLMSG_HDRLEN; } > Also, it seems there is at least one more instance of this same issue: > > diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c > index 16ae92054baa..d06184b94af5 100644 > --- a/net/netfilter/ipset/ip_set_core.c > +++ b/net/netfilter/ipset/ip_set_core.c > @@ -1723,7 +1723,8 @@ call_ad(struct net *net, struct sock *ctnl, struct sk_buff *skb, > nlh->nlmsg_seq, NLMSG_ERROR, payload, 0); > errmsg = nlmsg_data(rep); > errmsg->error = ret; > - memcpy(&errmsg->msg, nlh, nlh->nlmsg_len); > + errmsg->msg = *nlh; > + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload, nlmsg_len(nlh)); Ah, yes, nice catch! > cmdattr = (void *)&errmsg->msg + min_len; > > ret = nla_parse(cda, IPSET_ATTR_CMD_MAX, cmdattr, > > -- > Gustavo > > [1] https://elixir.bootlin.com/linux/v5.18-rc5/source/include/net/netlink.h#L577
Kees Cook <keescook@chromium.org> writes: > As part of the work to perform bounds checking on all memcpy() uses, > replace the open-coded a deserialization of bytes out of memory into a > trailing flexible array by using a flex_array.h helper to perform the > allocation, bounds checking, and copying. > > Cc: Loic Poulain <loic.poulain@linaro.org> > Cc: Kalle Valo <kvalo@kernel.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: wcn36xx@lists.infradead.org > Cc: linux-wireless@vger.kernel.org > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> [...] > --- a/drivers/net/wireless/ath/wcn36xx/smd.h > +++ b/drivers/net/wireless/ath/wcn36xx/smd.h > @@ -46,8 +46,8 @@ struct wcn36xx_fw_msg_status_rsp { > > struct wcn36xx_hal_ind_msg { > struct list_head list; > - size_t msg_len; > - u8 msg[]; > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, msg_len); > + DECLARE_FLEX_ARRAY_ELEMENTS(u8, msg); This affects readability quite a lot and tbh I don't like it. Isn't there any simpler way to solve this?
On Tue, 2022-05-03 at 18:44 -0700, Kees Cook wrote: > > For example, using the most complicated helper, mem_to_flex_dup(): > > /* Flexible array struct with members identified. */ > struct something { > int mode; > DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, how_many); > unsigned long flags; > DECLARE_FLEX_ARRAY_ELEMENTS(u32, value); In many cases, the order of the elements doesn't really matter, so maybe it'd be nicer to be able to write it as something like DECLARE_FLEX_STRUCT(something, int mode; unsigned long flags; , int, how_many, u32, value); perhaps? OK, that doesn't seem so nice either. Maybe struct something { int mode; unsigned long flags; FLEX_ARRAY( int, how_many, u32, value ); }; or so? The long and duplicated DECLARE_FLEX_ARRAY_ELEMENTS_COUNT and DECLARE_FLEX_ARRAY_ELEMENTS seems a bit tedious to me, at least in cases where the struct layout is not the most important thing (or it's already at the end anyway). > struct something *instance = NULL; > int rc; > > rc = mem_to_flex_dup(&instance, byte_array, count, GFP_KERNEL); > if (rc) > return rc; This seems rather awkward, having to set it to NULL, then checking rc (and possibly needing a separate variable for it), etc. But I can understand how you arrived at this: - need to pass instance or &instance or such for typeof() or offsetof() or such - instance = mem_to_flex_dup(instance, ...) looks too much like it would actually dup 'instance', rather than 'byte_array' - if you pass &instance anyway, checking for NULL is simple and adds a bit of safety but still, honestly, I don't like it. As APIs go, it feels a bit cumbersome and awkward to use, and you really need everyone to use this, and not say "uh what, I'll memcpy() instead". Maybe there should also be a realloc() version of it? > +/** __fas_bytes - Calculate potential size of flexible array structure I think you forgot "\n *" in many cases here after "/**". johannes
On Tue, 2022-05-03 at 18:44 -0700, Kees Cook wrote: > > @@ -2277,7 +2274,7 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy, > size_t ielen = len - offsetof(struct ieee80211_mgmt, > u.probe_resp.variable); > size_t new_ie_len; > - struct cfg80211_bss_ies *new_ies; > + struct cfg80211_bss_ies *new_ies = NULL; > const struct cfg80211_bss_ies *old; > u8 cpy_len; > > @@ -2314,8 +2311,7 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy, > if (!new_ie) > return; > > - new_ies = kzalloc(sizeof(*new_ies) + new_ie_len, GFP_ATOMIC); > - if (!new_ies) > + if (mem_to_flex_dup(&new_ies, new_ie, new_ie_len, GFP_ATOMIC)) > goto out_free; > > pos = new_ie; > @@ -2333,10 +2329,8 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy, > memcpy(pos, mbssid + cpy_len, ((ie + ielen) - (mbssid + cpy_len))); > > /* update ie */ > - new_ies->len = new_ie_len; > new_ies->tsf = le64_to_cpu(mgmt->u.probe_resp.timestamp); > new_ies->from_beacon = ieee80211_is_beacon(mgmt->frame_control); > - memcpy(new_ies->data, new_ie, new_ie_len); This introduces a bug, "new_ie" is modified between the kzalloc() and the memcpy(), but you've moved the memcpy() into the allocation. In fact, new_ie is completely freshly kzalloc()'ed at this point. So you need to change the ordering here, but since new_ie is freed pretty much immediately, we can probably just build the stuff directly inside new_ies->data, though then of course we cannot use your helper anymore? johannes
On Wed, May 04, 2022 at 08:42:46AM +0300, Kalle Valo wrote: > Kees Cook <keescook@chromium.org> writes: > > > As part of the work to perform bounds checking on all memcpy() uses, > > replace the open-coded a deserialization of bytes out of memory into a > > trailing flexible array by using a flex_array.h helper to perform the > > allocation, bounds checking, and copying. > > > > Cc: Loic Poulain <loic.poulain@linaro.org> > > Cc: Kalle Valo <kvalo@kernel.org> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Eric Dumazet <edumazet@google.com> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: Paolo Abeni <pabeni@redhat.com> > > Cc: wcn36xx@lists.infradead.org > > Cc: linux-wireless@vger.kernel.org > > Cc: netdev@vger.kernel.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > [...] > > > --- a/drivers/net/wireless/ath/wcn36xx/smd.h > > +++ b/drivers/net/wireless/ath/wcn36xx/smd.h > > @@ -46,8 +46,8 @@ struct wcn36xx_fw_msg_status_rsp { > > > > struct wcn36xx_hal_ind_msg { > > struct list_head list; > > - size_t msg_len; > > - u8 msg[]; > > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, msg_len); > > + DECLARE_FLEX_ARRAY_ELEMENTS(u8, msg); > > This affects readability quite a lot and tbh I don't like it. Isn't > there any simpler way to solve this? Similar to how I plumbed member names into __mem_to_flex(), I could do the same for __mem_to_flex_dup(). That way if the struct member aliases (DECLARE_FLEX...) aren't added, the longer form of the helper could be used. Instead of: if (mem_to_flex_dup(&msg_ind, buf, len, GFP_ATOMIC)) { it would be: if (__mem_to_flex_dup(&msg_ind, /* self */, msg, msg_len, buf, len, GFP_ATOMIC)) { This was how I'd written the helpers in an earlier version, but it seemed much cleaner to avoid repeating structure layout details at each call site. I couldn't find any other way to encode the needed information. It'd be wonderful if C would let us do: struct wcn36xx_hal_ind_msg { struct list_head list; size_t msg_len; u8 msg[msg_len]; } And provide some kind of interrogation: __builtin_flex_array_member(msg_ind) -> msg_ind->msg __builtin_flex_array_count(msg_ind) -> msg_ind->msg_len My hope would be to actually use the member aliases to teach things like -fsanitize=array-bounds about flexible arrays. If it encounters a structure with the aliases, it could add the instrumentation to do the bounds checking of things like: msg_ind->msg[42]; /* check that 42 is < msg_ind->msg_len */ I also wish I could find a way to make the proposed macros "forward portable" into proposed C syntax above, but this eluded me as well. For example: struct wcn36xx_hal_ind_msg { size_t msg_len; struct list_head list; BOUNDED_FLEX_ARRAY(u8, msg, msg_len); } #ifdef CC_HAS_DYNAMIC_ARRAY_LEN # define BOUNDED_FLEX_ARRAY(type, name, bounds) type name[bounds] #else # define BOUNDED_FLEX_ARRAY(type, name, bounds) \ magic_alias_of msg_len __flex_array_elements_count; \ union { \ type name[]; \ type __flex_array_elements[]; \ } #endif But I couldn't sort out the "magic_alias_of" syntax that wouldn't force structures into having the count member immediately before the flex array, which would impose more limitations on where this could be used... Anyway, I'm open to ideas on how to improve this! -Kees
On Wed, May 04, 2022 at 09:28:46AM +0200, Johannes Berg wrote: > On Tue, 2022-05-03 at 18:44 -0700, Kees Cook wrote: > > > > @@ -2277,7 +2274,7 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy, > > size_t ielen = len - offsetof(struct ieee80211_mgmt, > > u.probe_resp.variable); > > size_t new_ie_len; > > - struct cfg80211_bss_ies *new_ies; > > + struct cfg80211_bss_ies *new_ies = NULL; > > const struct cfg80211_bss_ies *old; > > u8 cpy_len; > > > > @@ -2314,8 +2311,7 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy, > > if (!new_ie) > > return; > > > > - new_ies = kzalloc(sizeof(*new_ies) + new_ie_len, GFP_ATOMIC); > > - if (!new_ies) > > + if (mem_to_flex_dup(&new_ies, new_ie, new_ie_len, GFP_ATOMIC)) > > goto out_free; > > > > pos = new_ie; > > @@ -2333,10 +2329,8 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy, > > memcpy(pos, mbssid + cpy_len, ((ie + ielen) - (mbssid + cpy_len))); > > > > /* update ie */ > > - new_ies->len = new_ie_len; > > new_ies->tsf = le64_to_cpu(mgmt->u.probe_resp.timestamp); > > new_ies->from_beacon = ieee80211_is_beacon(mgmt->frame_control); > > - memcpy(new_ies->data, new_ie, new_ie_len); > > This introduces a bug, "new_ie" is modified between the kzalloc() and > the memcpy(), but you've moved the memcpy() into the allocation. In > fact, new_ie is completely freshly kzalloc()'ed at this point. So you > need to change the ordering here, but since new_ie is freed pretty much > immediately, we can probably just build the stuff directly inside > new_ies->data, though then of course we cannot use your helper anymore? Eek, yes, thanks. My attempt to locate the alloc/memcpy pattern failed to take into account anything touch the source between alloc and memcpy. I'll double check the other examples. -Kees
On Tue, May 03, 2022 at 06:44:29PM -0700, Kees Cook wrote: > As part of the work to perform bounds checking on all memcpy() uses, > replace the open-coded a deserialization of bytes out of memory into a > trailing flexible array by using a flex_array.h helper to perform the > allocation, bounds checking, and copying. Acked-by: Mark Brown <broonie@kernel.org>
On Wed, May 04, 2022 at 09:25:56AM +0200, Johannes Berg wrote: > On Tue, 2022-05-03 at 18:44 -0700, Kees Cook wrote: > > > > For example, using the most complicated helper, mem_to_flex_dup(): > > > > /* Flexible array struct with members identified. */ > > struct something { > > int mode; > > DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, how_many); > > unsigned long flags; > > DECLARE_FLEX_ARRAY_ELEMENTS(u32, value); > > In many cases, the order of the elements doesn't really matter, so maybe > it'd be nicer to be able to write it as something like > > DECLARE_FLEX_STRUCT(something, > int mode; > unsigned long flags; > , > int, how_many, > u32, value); > > perhaps? OK, that doesn't seem so nice either. > > Maybe > > struct something { > int mode; > unsigned long flags; > FLEX_ARRAY( > int, how_many, > u32, value > ); > }; Yeah, I mention some of my exploration of this idea in the sibling reply: https://lore.kernel.org/linux-hardening/202205040730.161645EC@keescook/#t It seemed like requiring a structure be rearranged to take advantage of the "automatic layout introspection" wasn't very friendly. On the other hand, looking at the examples, most of them are already neighboring members. Hmmm. > or so? The long and duplicated DECLARE_FLEX_ARRAY_ELEMENTS_COUNT and > DECLARE_FLEX_ARRAY_ELEMENTS seems a bit tedious to me, at least in cases > where the struct layout is not the most important thing (or it's already > at the end anyway). The names aren't great, but I wanted to distinguish "elements" as the array not the count. Yay naming. However, perhaps the solution is to have _both_. i.e using BOUNDED_FLEX_ARRAY(count_type, count_name, array_type, array_name) for the "neighboring" case, and the DECLARE...{ELEMENTS,COUNT} for the "split" case. And DECLARE_FLEX_ARRAY_ELEMENTS could actually be expanded to include the count_name too, so both methods could be "forward portable" to a future where C grew the syntax for bounded flex arrays. > > > struct something *instance = NULL; > > int rc; > > > > rc = mem_to_flex_dup(&instance, byte_array, count, GFP_KERNEL); > > if (rc) > > return rc; > > This seems rather awkward, having to set it to NULL, then checking rc > (and possibly needing a separate variable for it), etc. I think the errno return is completely required. I had an earlier version of this that was much more like a drop-in replacement for memcpy that would just truncate or panic, and when I had it all together, I could just imagine hearing Linus telling me to start over because it was unsafe (truncation may be just as bad as overflow) and disruptive ("never BUG"), and that it should be recoverable. So, I rewrote it all to return a __must_check errno. Requiring instance to be NULL is debatable, but I feel pretty strongly about it because it does handle a class of mistakes (resource leaks), and it's not much of a burden to require a known-good starting state. > But I can understand how you arrived at this: > - need to pass instance or &instance or such for typeof() > or offsetof() or such Right. > - instance = mem_to_flex_dup(instance, ...) > looks too much like it would actually dup 'instance', rather than > 'byte_array' And I need an errno output to keep imaginary Linus happy. :) > - if you pass &instance anyway, checking for NULL is simple and adds a > bit of safety Right. > but still, honestly, I don't like it. As APIs go, it feels a bit > cumbersome and awkward to use, and you really need everyone to use this, > and not say "uh what, I'll memcpy() instead". Sure, and I have tried to get it down as small as possible. The earlier "just put all the member names in every call" version was horrid. :P I realize it's more work to check errno, but the memcpy() API we've all been trained to use is just plain dangerous. I don't think it's unreasonable to ask people to retrain themselves to avoid it. All that said, yes, I want it to be as friendly as possible. > Maybe there should also be a realloc() version of it? Sure! Seems reasonable. I'd like to see the code pattern for this though. Do you have any examples? Most of what I'd been able to find for the fragile memcpy() usage was just basic serialize/deserialize or direct copying. > > +/** __fas_bytes - Calculate potential size of flexible array structure > > I think you forgot "\n *" in many cases here after "/**". Oops! Yes, thank you. I'll fix these. -Kees
From: Kees Cook > Sent: 04 May 2022 16:38 ... > > > struct something *instance = NULL; > > > int rc; > > > > > > rc = mem_to_flex_dup(&instance, byte_array, count, GFP_KERNEL); > > > if (rc) > > > return rc; > > > > This seems rather awkward, having to set it to NULL, then checking rc > > (and possibly needing a separate variable for it), etc. > > I think the errno return is completely required. I had an earlier version > of this that was much more like a drop-in replacement for memcpy that > would just truncate or panic, and when I had it all together, I could > just imagine hearing Linus telling me to start over because it was unsafe > (truncation may be just as bad as overflow) and disruptive ("never BUG"), > and that it should be recoverable. So, I rewrote it all to return a > __must_check errno. > > Requiring instance to be NULL is debatable, but I feel pretty strongly > about it because it does handle a class of mistakes (resource leaks), > and it's not much of a burden to require a known-good starting state. Why not make it look like malloc() since it seems to be malloc(). That gives a much better calling convention. Passing pointers and integers by reference can generate horrid code. (Mostly because it stops the compiler keeping values in registers.) If you want the type information inside the 'function' use a #define so that the use is: mem_to_flex_dup(instance, byte_array, count, GFP_KERNEL); if (!instance) return ... (or use ERR_PTR() etc). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, May 04, 2022 at 11:00:38AM +0800, David Gow wrote: > On Wed, May 4, 2022 at 9:47 AM Kees Cook <keescook@chromium.org> wrote: > > > > Add tests for the new flexible array structure helpers. These can be run > > with: > > > > make ARCH=um mrproper > > ./tools/testing/kunit/kunit.py config > > Nit: it shouldn't be necessary to run kunit.py config separately: > kunit.py run will configure the kernel if necessary. Ah yes, I think you mentioned this before. I'll adjust the commit log. > > > ./tools/testing/kunit/kunit.py run flex_array > > > > Cc: David Gow <davidgow@google.com> > > Cc: kunit-dev@googlegroups.com > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > This looks pretty good to me: it certainly worked on the different > setups I tried (um, x86_64, x86_64+KASAN). > > A few minor nitpicks inline, mostly around minor config-y things, or > things which weren't totally clear on my first read-through. > > Hopefully one day, with the various stubbing features or something > similar, we'll be able to check against allocation failures in > flex_dup(), too, but otherwise nothing seems too obviously missing. > > Reviewed-by: David Gow <davidgow@google.com> Great; thanks for the review and testing! > > -- David > > > lib/Kconfig.debug | 12 +- > > lib/Makefile | 1 + > > lib/flex_array_kunit.c | 523 +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 531 insertions(+), 5 deletions(-) > > create mode 100644 lib/flex_array_kunit.c > > > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > index 9077bb38bc93..8bae6b169c50 100644 > > --- a/lib/Kconfig.debug > > +++ b/lib/Kconfig.debug > > @@ -2551,11 +2551,6 @@ config OVERFLOW_KUNIT_TEST > > Builds unit tests for the check_*_overflow(), size_*(), allocation, and > > related functions. > > > > - For more information on KUnit and unit tests in general please refer > > - to the KUnit documentation in Documentation/dev-tools/kunit/. > > - > > - If unsure, say N. > > - > > Nit: while I'm not against removing some of this boilerplate, is it > better suited for a separate commit? Make sense, yes. I'll drop this for now. > > > config STACKINIT_KUNIT_TEST > > tristate "Test level of stack variable initialization" if !KUNIT_ALL_TESTS > > depends on KUNIT > > @@ -2567,6 +2562,13 @@ config STACKINIT_KUNIT_TEST > > CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF, > > or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. > > > > +config FLEX_ARRAY_KUNIT_TEST > > + tristate "Test flex_*() family of helper functions at runtime" if !KUNIT_ALL_TESTS > > + depends on KUNIT > > + default KUNIT_ALL_TESTS > > + help > > + Builds unit tests for flexible array copy helper functions. > > + > > Nit: checkpatch warns that the description here may be insufficient: > WARNING: please write a help paragraph that fully describes the config symbol Yeah, I don't know anything to put here that isn't just more boilerplate, so I'm choosing to ignore this for now. :) > > [...] > > +struct normal { > > + size_t datalen; > > + u32 data[]; > > +}; > > + > > +struct decl_normal { > > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, datalen); > > + DECLARE_FLEX_ARRAY_ELEMENTS(u32, data); > > +}; > > + > > +struct aligned { > > + unsigned short datalen; > > + char data[] __aligned(__alignof__(u64)); > > +}; > > + > > +struct decl_aligned { > > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(unsigned short, datalen); > > + DECLARE_FLEX_ARRAY_ELEMENTS(char, data) __aligned(__alignof__(u64)); > > +}; > > + > > +static void struct_test(struct kunit *test) > > +{ > > + COMPARE_STRUCTS(struct normal, struct decl_normal); > > + COMPARE_STRUCTS(struct aligned, struct decl_aligned); > > +} > > If I understand it, the purpose of this is to ensure that structs both > with and without the flexible array declaration have the same memory > layout? > > If so, any chance of a comment briefly stating that's the purpose (or > renaming this test struct_layout_test())? Yeah, good idea; I'll improve the naming. > > Also, would it make sense to do the same with the struct with internal > padding below? Heh, yes, good point! :) > [...] > > +#define CHECK_COPY(ptr) do { \ > > + typeof(*(ptr)) *_cc_dst = (ptr); \ > > + KUNIT_EXPECT_EQ(test, _cc_dst->induce_padding, 0); \ > > + memcpy(&padding, &_cc_dst->induce_padding + sizeof(_cc_dst->induce_padding), \ > > + sizeof(padding)); \ > > + /* Padding should be zero too. */ \ > > + KUNIT_EXPECT_EQ(test, padding, 0); \ > > + KUNIT_EXPECT_EQ(test, src->count, _cc_dst->count); \ > > + KUNIT_EXPECT_EQ(test, _cc_dst->count, TEST_TARGET); \ > > + for (i = 0; i < _cc_dst->count - 1; i++) { \ > > + /* 'A' is 0x41, and here repeated in a u32. */ \ > > Would it be simpler to just note that the magic value is 0x41, rather > than have it be the character 'A'? Yeah, now fixed. > [...] > > + CHECK_COPY(&encap->fas); > > + /* Check that items external to "fas" are zero. */ > > + KUNIT_EXPECT_EQ(test, encap->flags, 0); > > + KUNIT_EXPECT_EQ(test, encap->junk, 0); > > + kfree(encap); > > +#undef MAGIC_WORD > > MAGIC_WORD isn't defined (or used) for flux_dup_test? Is it worth > using it (or something similar) for the 'A' / 0x14141414 and the > CHECK_COPY() macro? Oops, yes. Fixed. Thanks again! -Kees
On Tue, May 3, 2022 at 8:47 PM Kees Cook <keescook@chromium.org> wrote: > +#define COMPARE_STRUCTS(STRUCT_A, STRUCT_B) do { \ > + STRUCT_A *ptr_A; \ > + STRUCT_B *ptr_B; \ > + int rc; \ > + size_t size_A, size_B; \ > + \ > + /* matching types for flex array elements and count */ \ > + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A), sizeof(*ptr_B)); \ > + KUNIT_EXPECT_TRUE(test, __same_type(*ptr_A->data, \ > + *ptr_B->__flex_array_elements)); \ Leaving some minor suggestions to go along with David's comments. Should we make these KUNIT_ASSERT_.* instead? I assume if we have a type-mismatch, then we should bail out instead of continuing to produce more error messages. > + KUNIT_EXPECT_TRUE(test, __same_type(ptr_A->datalen, \ > + ptr_B->__flex_array_elements_count)); \ > + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A->data), \ > + sizeof(*ptr_B->__flex_array_elements)); \ > + KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), data), \ > + offsetof(typeof(*ptr_B), \ > + __flex_array_elements)); \ > + KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), datalen), \ > + offsetof(typeof(*ptr_B), \ > + __flex_array_elements_count)); \ > + \ > + /* struct_size() vs __fas_bytes() */ \ > + size_A = struct_size(ptr_A, data, 13); \ > + rc = __fas_bytes(ptr_B, __flex_array_elements, \ > + __flex_array_elements_count, 13, &size_B); \ > + KUNIT_EXPECT_EQ(test, rc, 0); \ Hmm, what do you think about inlining the call/dropping rc? i.e. something like KUNIT_EXPECT_EQ(test, 0, __fas_bytes(ptr_B, __flex_array_elements, \ __flex_array_elements_count, 13, &size_B)); That would give a slightly clearer error message on failure. Otherwise the user only really gets a line number to try and start to understand what went wrong. > + > +#define CHECK_COPY(ptr) do { \ > + typeof(*(ptr)) *_cc_dst = (ptr); \ > + KUNIT_EXPECT_EQ(test, _cc_dst->induce_padding, 0); \ > + memcpy(&padding, &_cc_dst->induce_padding + sizeof(_cc_dst->induce_padding), \ > + sizeof(padding)); \ > + /* Padding should be zero too. */ \ > + KUNIT_EXPECT_EQ(test, padding, 0); \ > + KUNIT_EXPECT_EQ(test, src->count, _cc_dst->count); \ This also seems like a good place to use ASSERT instead of EXPECT. > + KUNIT_EXPECT_EQ(test, _cc_dst->count, TEST_TARGET); \ > + for (i = 0; i < _cc_dst->count - 1; i++) { \ > + /* 'A' is 0x41, and here repeated in a u32. */ \ > + KUNIT_EXPECT_EQ(test, _cc_dst->flex[i], 0x41414141); \ > + } \ > + /* Last item should be different. */ \ > + KUNIT_EXPECT_EQ(test, _cc_dst->flex[_cc_dst->count - 1], 0x14141414); \ > +} while (0) > + > +/* Test copying from one flexible array struct into another. */ > +static void flex_cpy_test(struct kunit *test) > +{ > +#define TEST_BOUNDS 13 > +#define TEST_TARGET 12 > +#define TEST_SMALL 10 > + struct flex_cpy_obj *src, *dst; > + unsigned long padding; > + int i, rc; > + > + /* Prepare open-coded source. */ > + src = kzalloc(struct_size(src, flex, TEST_BOUNDS), GFP_KERNEL); Looks like we could use kunit_kzalloc() here and avoid needing the manual call to kfree? This also holds for the other test cases where they don't have early calls to kfree(). Doing so would also let you use KUNIT_ASSERT's without fear of leaking these allocations. > + src->count = TEST_BOUNDS; > + memset(src->flex, 'A', flex_array_size(src, flex, TEST_BOUNDS)); > + src->flex[src->count - 2] = 0x14141414; > + src->flex[src->count - 1] = 0x24242424; > + > + /* Prepare open-coded destination, alloc only. */ > + dst = kzalloc(struct_size(src, flex, TEST_BOUNDS), GFP_KERNEL); > + /* Pre-fill with 0xFE marker. */ > + memset(dst, 0xFE, struct_size(src, flex, TEST_BOUNDS)); > + /* Pretend we're 1 element smaller. */ > + dst->count = TEST_TARGET; > + > + /* Pretend to match the target destination size. */ > + src->count = TEST_TARGET; > + > + rc = flex_cpy(dst, src); > + KUNIT_EXPECT_EQ(test, rc, 0); > + CHECK_COPY(dst); > + /* Item past last copied item is unchanged from initial memset. */ > + KUNIT_EXPECT_EQ(test, dst->flex[dst->count], 0xFEFEFEFE); > + > + /* Now trip overflow, and verify we didn't clobber beyond end. */ > + src->count = TEST_BOUNDS; > + rc = flex_cpy(dst, src); > + KUNIT_EXPECT_EQ(test, rc, -E2BIG); > + /* Item past last copied item is unchanged from initial memset. */ > + KUNIT_EXPECT_EQ(test, dst->flex[dst->count], 0xFEFEFEFE); > + > + /* Reset destination contents. */ > + memset(dst, 0xFD, struct_size(src, flex, TEST_BOUNDS)); > + dst->count = TEST_TARGET; > + > + /* Copy less than max. */ > + src->count = TEST_SMALL; > + rc = flex_cpy(dst, src); > + KUNIT_EXPECT_EQ(test, rc, 0); > + /* Verify count was adjusted. */ > + KUNIT_EXPECT_EQ(test, dst->count, TEST_SMALL); Just an FYI, macros get evaluated before the expect macros can stringify them. So the error message would look something like Expected dest->count == 10 but dest->count = 9 Not a big concern, but just noting that "TEST_SMALL" won't be visible at all. Could opt for KUNIT_EXPECT_EQ_MSG(test, dst->count, TEST_SMALL, "my custom extra message"); if you think it'd be usable to make the test more grokkable. > + /* Verify element beyond src size was wiped. */ > + KUNIT_EXPECT_EQ(test, dst->flex[TEST_SMALL], 0); > + /* Verify element beyond original dst size was untouched. */ > + KUNIT_EXPECT_EQ(test, dst->flex[TEST_TARGET], 0xFDFDFDFD); > + > + kfree(dst); > + kfree(src); > +#undef TEST_BOUNDS > +#undef TEST_TARGET > +#undef TEST_SMALL > +} > + > +static void flex_dup_test(struct kunit *test) > +{ > +#define TEST_TARGET 12 > + struct flex_cpy_obj *src, *dst = NULL, **null = NULL; > + struct flex_dup_obj *encap = NULL; > + unsigned long padding; > + int i, rc; > + > + /* Prepare open-coded source. */ > + src = kzalloc(struct_size(src, flex, TEST_TARGET), GFP_KERNEL); > + src->count = TEST_TARGET; > + memset(src->flex, 'A', flex_array_size(src, flex, TEST_TARGET)); > + src->flex[src->count - 1] = 0x14141414; > + > + /* Reject NULL @alloc. */ > + rc = flex_dup(null, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + > + /* Check good copy. */ > + rc = flex_dup(&dst, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_ASSERT_TRUE(test, dst != NULL); > + CHECK_COPY(dst); > + > + /* Reject non-NULL *@alloc. */ > + rc = flex_dup(&dst, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, -EINVAL); > + > + kfree(dst); > + > + /* Check good encap copy. */ > + rc = __flex_dup(&encap, .fas, src, GFP_KERNEL); > + KUNIT_EXPECT_EQ(test, rc, 0); > + KUNIT_ASSERT_TRUE(test, dst != NULL); FYI, there's a new KUNIT_ASSERT_NOT_NULL() macro in the -kselftest/kunit branch, https://patchwork.kernel.org/project/linux-kselftest/patch/20220211164246.410079-1-ribalda@chromium.org/ But that's not planned for inclusion into mainline until 5.19, so leaving this as-is is better for now.
On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote: > > As part of the work to perform bounds checking on all memcpy() uses, > replace the open-coded a deserialization of bytes out of memory into a > trailing flexible array by using a flex_array.h helper to perform the > allocation, bounds checking, and copying: > > struct xfrm_sec_ctx > struct sidtab_str_cache > > Cc: Steffen Klassert <steffen.klassert@secunet.com> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Paul Moore <paul@paul-moore.com> > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > Cc: Eric Paris <eparis@parisplace.org> > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Xiu Jianfeng <xiujianfeng@huawei.com> > Cc: "Christian Göttsche" <cgzones@googlemail.com> > Cc: netdev@vger.kernel.org > Cc: selinux@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/uapi/linux/xfrm.h | 4 ++-- > security/selinux/ss/sidtab.c | 9 +++------ > security/selinux/xfrm.c | 7 ++----- > 3 files changed, 7 insertions(+), 13 deletions(-) > > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h > index 65e13a099b1a..4a6fa2beff6a 100644 > --- a/include/uapi/linux/xfrm.h > +++ b/include/uapi/linux/xfrm.h > @@ -31,9 +31,9 @@ struct xfrm_id { > struct xfrm_sec_ctx { > __u8 ctx_doi; > __u8 ctx_alg; > - __u16 ctx_len; > + __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len); > __u32 ctx_sid; > - char ctx_str[0]; > + __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str); > }; While I like the idea of this in principle, I'd like to hear about the testing you've done on these patches. A previous flex array conversion in the audit uapi headers ended up causing a problem with GCC12 and SWIG; while it was a SWIG problem and not a kernel header problem that was thin consolation for those with broken builds. > diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c > index a54b8652bfb5..a9d434e8cff7 100644 > --- a/security/selinux/ss/sidtab.c > +++ b/security/selinux/ss/sidtab.c > @@ -23,8 +23,8 @@ struct sidtab_str_cache { > struct rcu_head rcu_member; > struct list_head lru_member; > struct sidtab_entry *parent; > - u32 len; > - char str[]; > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, len); > + DECLARE_FLEX_ARRAY_ELEMENTS(char, str); > }; > > #define index_to_sid(index) ((index) + SECINITSID_NUM + 1)
Hi Paul, On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote: > On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote: [..] > > +++ b/include/uapi/linux/xfrm.h > > @@ -31,9 +31,9 @@ struct xfrm_id { > > struct xfrm_sec_ctx { > > __u8 ctx_doi; > > __u8 ctx_alg; > > - __u16 ctx_len; > > + __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len); > > __u32 ctx_sid; > > - char ctx_str[0]; > > + __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str); > > }; > > While I like the idea of this in principle, I'd like to hear about the > testing you've done on these patches. A previous flex array > conversion in the audit uapi headers ended up causing a problem with I'm curious about which commit caused those problems...? Thanks -- Gustavo > GCC12 and SWIG; while it was a SWIG problem and not a kernel header > problem that was thin consolation for those with broken builds.
On Wed, May 4, 2022 at 7:34 PM Gustavo A. R. Silva <gustavoars@kernel.org> wrote: > > Hi Paul, > > On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote: > > On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote: > > [..] > > > > +++ b/include/uapi/linux/xfrm.h > > > @@ -31,9 +31,9 @@ struct xfrm_id { > > > struct xfrm_sec_ctx { > > > __u8 ctx_doi; > > > __u8 ctx_alg; > > > - __u16 ctx_len; > > > + __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len); > > > __u32 ctx_sid; > > > - char ctx_str[0]; > > > + __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str); > > > }; > > > > While I like the idea of this in principle, I'd like to hear about the > > testing you've done on these patches. A previous flex array > > conversion in the audit uapi headers ended up causing a problem with > > I'm curious about which commit caused those problems...? Commit ed98ea2128b6 ("audit: replace zero-length array with flexible-array member"), however, as I said earlier, the problem was actually with SWIG, it just happened to be triggered by the kernel commit. There was a brief fedora-devel mail thread about the problem, see the link below: * https://www.spinics.net/lists/fedora-devel/msg297991.html To reiterate, I'm supportive of changes like this, but I would like to hear how it was tested to ensure there are no unexpected problems with userspace. If there are userspace problems it doesn't mean we can't make changes like this, it just means we need to ensure that the userspace issues are resolved first.
On Wed, 2022-05-04 at 08:38 -0700, Kees Cook wrote: > > It seemed like requiring a structure be rearranged to take advantage of > the "automatic layout introspection" wasn't very friendly. On the other > hand, looking at the examples, most of them are already neighboring > members. Hmmm. A lot of them are, and many could be, though not all. > > or so? The long and duplicated DECLARE_FLEX_ARRAY_ELEMENTS_COUNT and > > DECLARE_FLEX_ARRAY_ELEMENTS seems a bit tedious to me, at least in cases > > where the struct layout is not the most important thing (or it's already > > at the end anyway). > > The names aren't great, but I wanted to distinguish "elements" as the > array not the count. Yay naming. :-) > However, perhaps the solution is to have _both_. i.e using > BOUNDED_FLEX_ARRAY(count_type, count_name, array_type, array_name) for > the "neighboring" case, and the DECLARE...{ELEMENTS,COUNT} for the > "split" case. Seems reasonable to me. > And DECLARE_FLEX_ARRAY_ELEMENTS could actually be expanded to include > the count_name too, so both methods could be "forward portable" to a > future where C grew the syntax for bounded flex arrays. I guess I don't see that happening :) > > This seems rather awkward, having to set it to NULL, then checking rc > > (and possibly needing a separate variable for it), etc. > > I think the errno return is completely required. I had an earlier version > of this that was much more like a drop-in replacement for memcpy that > would just truncate or panic, > Oh, I didn't mean to imply it should truncate or panic or such - but if it returns a pointer it can still be an ERR_PTR() or NULL instead of having this separate indication, which even often confuses static type checkers since they don't always see the "errno == 0 <=> ptr != NULL" relation. So not saying you shouldn't have any error return - clearly you need that, just saying that I'm not sure that having the two separated is great. > Requiring instance to be NULL is debatable, but I feel pretty strongly > about it because it does handle a class of mistakes (resource leaks), > and it's not much of a burden to require a known-good starting state. Yeah, dunno, I guess I'm slightly more on the side of not requiring it, since we don't do the same for kmalloc() etc. and probably really wouldn't want to add kmalloc_s() that does it ;-) I mean, you _could_ go there: int kmalloc_s(void **ptr, size_t size, gfp_t gfp) { void *ret; if (*ptr) return -EINVAL; ret = kmalloc(size, gfp); if (!ret) return -ENOMEM; *ptr = ret; return 0; } right? But we don't really do that, and I'm not sure it'd be a win if done over the whole code base. So I'm not really sure why this aspect here should need to be different, except of course that you already need the input argument for the magic. But we could still have (this prototype is theoretical, of course, it cannot be implemented in C): void *mem_to_flex_dup(void *ptr, const void *data, size_t elements, gfp_t gfp); which isn't really that much better though. And btw, while I was writing it down I was looking to see if it should be "size_t elements" or "size_t len" (like memcpy), it took me some time to figure out, and I was looking at the examples: 1) most of them actually use __u8 or some variant thereof, so you could probably add an even simpler macro like BOUNDED_FLEX_DATA(int, bytes, data) which has the u8 type internally. 2) Unless I'm confusing myself, you got the firewire change wrong, because __mem_to_flex_dup takes the "elements_count", but the memcpy() there wasn't multiplied by the sizeof(element)? Or maybe the fact that it was declared as __u32 header[0] is wrong, and it should be __u8, but it's all very confusing, and I'm really not sure about this at all. One "perhaps you'll laugh me out of the room" suggestion might be to actually be able to initialize the whole thing too? mydata = flex_struct_alloc(mydata, GFP_KERNEL, variable_data, variable_len, .member = 1, .another = 2); (the ordering can't really be otherwise since you have to use __VA_ARGS__). That might reduce some more code too, though I guess it's quite some additional magic ... :) > > but still, honestly, I don't like it. As APIs go, it feels a bit > > cumbersome and awkward to use, and you really need everyone to use this, > > and not say "uh what, I'll memcpy() instead". > > Sure, and I have tried to get it down as small as possible. The earlier > "just put all the member names in every call" version was horrid. :P :-D > I > realize it's more work to check errno, but the memcpy() API we've all > been trained to use is just plain dangerous. I don't think it's > unreasonable to ask people to retrain themselves to avoid it. All that > said, yes, I want it to be as friendly as possible. > > > Maybe there should also be a realloc() version of it? > > Sure! Seems reasonable. I'd like to see the code pattern for this > though. Do you have any examples? I was going to point to struct cfg80211_bss_ies, but I realize now they're RCU-managed, so we never resize them anyway ... So maybe it's less common than I thought it might be. I suppose you know better since you converted a lot of stuff already :-) johannes
Johannes Berg <johannes@sipsolutions.net> writes: > Yeah, dunno, I guess I'm slightly more on the side of not requiring it, > since we don't do the same for kmalloc() etc. and probably really > wouldn't want to add kmalloc_s() that does it ;-) I suspect the number of bugs this catches will be small, but they'll be in places where the flow of control is complicated. What we want is to know that there's no "real" value already present. I'd love it if we could make the macro declare a new name (yeah, I know, mixing declarations and code). Of course, we could also end up with people writing a wrapping macro that sets the variable to NULL before invoking the underlying macro...
On Wed, May 04, 2022 at 11:14:42PM -0400, Paul Moore wrote: > On Wed, May 4, 2022 at 7:34 PM Gustavo A. R. Silva > <gustavoars@kernel.org> wrote: > > > > Hi Paul, > > > > On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote: > > > On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote: > > > > [..] > > > > > > +++ b/include/uapi/linux/xfrm.h > > > > @@ -31,9 +31,9 @@ struct xfrm_id { > > > > struct xfrm_sec_ctx { > > > > __u8 ctx_doi; > > > > __u8 ctx_alg; > > > > - __u16 ctx_len; > > > > + __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len); > > > > __u32 ctx_sid; > > > > - char ctx_str[0]; > > > > + __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str); > > > > }; > > > > > > While I like the idea of this in principle, I'd like to hear about the > > > testing you've done on these patches. A previous flex array > > > conversion in the audit uapi headers ended up causing a problem with > > > > I'm curious about which commit caused those problems...? > > Commit ed98ea2128b6 ("audit: replace zero-length array with > flexible-array member"), however, as I said earlier, the problem was > actually with SWIG, it just happened to be triggered by the kernel > commit. There was a brief fedora-devel mail thread about the problem, > see the link below: > > * https://www.spinics.net/lists/fedora-devel/msg297991.html Wow, that's pretty weird -- it looks like SWIG was scraping the headers to build its conversions? I assume SWIG has been fixed now? > To reiterate, I'm supportive of changes like this, but I would like to > hear how it was tested to ensure there are no unexpected problems with > userspace. If there are userspace problems it doesn't mean we can't > make changes like this, it just means we need to ensure that the > userspace issues are resolved first. Well, as this is the first and only report of any problems with [0] -> [] conversions (in UAPI or anywhere) that I remember seeing, and they've been underway since at least v5.9, I hadn't been doing any new testing. So, for this case, I guess I should ask what tests you think would be meaningful here? Anything using #include should be fine: https://codesearch.debian.net/search?q=linux%2Fxfrm.h&literal=1&perpkg=1 Which leaves just this, which may be doing something weird: libabigail_2.0-1/tests/data/test-diff-filter/test-PR27569-v0.abi </data-member> <data-member access="public" layout-offset-in-bits="128"> <var-decl name="seq_hi" type-id="3f1a6b60" visibility="default" filepath="include/uapi/linux/xfrm.h" line="97" column="1"/> </data-member> <data-member access="public" layout-offset-in-bits="160"> But I see that SWIG doesn't show up in a search for linux/audit.h: https://codesearch.debian.net/search?q=linux%2Faudit.h&literal=1&perpkg=1 So this may not be a sufficient analysis...
On Thu, May 05, 2022 at 03:16:19PM +0200, Johannes Berg wrote: > On Wed, 2022-05-04 at 08:38 -0700, Kees Cook wrote: > > > > It seemed like requiring a structure be rearranged to take advantage of > > the "automatic layout introspection" wasn't very friendly. On the other > > hand, looking at the examples, most of them are already neighboring > > members. Hmmm. > > A lot of them are, and many could be, though not all. Yeah, I did a pass through them for the coming v2. Only a few have the struct order as part of an apparent hardware interface. > > And DECLARE_FLEX_ARRAY_ELEMENTS could actually be expanded to include > > the count_name too, so both methods could be "forward portable" to a > > future where C grew the syntax for bounded flex arrays. > > I guess I don't see that happening :) Well ... it's on my roadmap. ;) I want it for -fsanitize=array-bounds so that dynamic array indexing can be checked too. (Right now we can do constant-sized array index bounds checking at runtime, but the much harder to find problems tend to come from flex arrays.) > > Requiring instance to be NULL is debatable, but I feel pretty strongly > > about it because it does handle a class of mistakes (resource leaks), > > and it's not much of a burden to require a known-good starting state. > > Yeah, dunno, I guess I'm slightly more on the side of not requiring it, > since we don't do the same for kmalloc() etc. and probably really > wouldn't want to add kmalloc_s() that does it ;-) Well, I dislike all the *alloc APIs. :P > I mean, you _could_ go there: > > int kmalloc_s(void **ptr, size_t size, gfp_t gfp) Oh, and I really do (though as a macro, not a "real" function), since having type introspection would be _extremely_ useful. Though maybe it needs to be through some kind of type-of-lvalue thing... https://github.com/KSPP/linux/issues/189 https://github.com/KSPP/linux/issues/87 > So I'm not really sure why this aspect here should need to be different, > except of course that you already need the input argument for the magic. Right, and trying to move the kernel code closer to a form where the compiler can take more of the burden of handling code safety. > And btw, while I was writing it down I was looking to see if it should > be "size_t elements" or "size_t len" (like memcpy), it took me some time > to figure out, and I was looking at the examples: > > 1) most of them actually use __u8 or some variant thereof, so you > could probably add an even simpler macro like > BOUNDED_FLEX_DATA(int, bytes, data) > which has the u8 type internally. I didn't want these helpers to be "opinionated" about their types (just their API), so while it's true u8 is usually "good enough", I don't think it's common enough to make a special case for. > 2) Unless I'm confusing myself, you got the firewire change wrong, > because __mem_to_flex_dup takes the "elements_count", but the > memcpy() there wasn't multiplied by the sizeof(element)? Or maybe > the fact that it was declared as __u32 header[0] is wrong, and it > should be __u8, but it's all very confusing, and I'm really not > sure about this at all. Yes indeed; thanks for catching that. In fact, it's not a strict flex array struct, since, as you say, it's measuring bytes, not elements. Yeah, I'll see if that needs to be adjusted/dropped, etc. > One "perhaps you'll laugh me out of the room" suggestion might be to > actually be able to initialize the whole thing too? > > mydata = flex_struct_alloc(mydata, GFP_KERNEL, > variable_data, variable_len, > .member = 1, > .another = 2); > > (the ordering can't really be otherwise since you have to use > __VA_ARGS__). Oooh, that's a cool idea for the API. Hmmmm. > That might reduce some more code too, though I guess it's quite some > additional magic ... :) Yay preprocessor magic! > I was going to point to struct cfg80211_bss_ies, but I realize now > they're RCU-managed, so we never resize them anyway ... So maybe it's > less common than I thought it might be. > > I suppose you know better since you converted a lot of stuff already :-) Well, I've seen a lot of fragile code (usually in the form of exploitable flaws around flex arrays) and they do mostly look the same. Not everything fits perfectly into the forms this API tries to address, but my goal is to get it fitting well enough, and the weird stuff can be more carefully examined -- they're easier to find and audit if all the others are nicely wrapped up in some fancy flex*() API. Thanks for your thoughts on all of this! I'll continue to work on a v2... -Kees
On Thu, May 05, 2022 at 08:16:11AM -0700, Keith Packard wrote: > Johannes Berg <johannes@sipsolutions.net> writes: > > > Yeah, dunno, I guess I'm slightly more on the side of not requiring it, > > since we don't do the same for kmalloc() etc. and probably really > > wouldn't want to add kmalloc_s() that does it ;-) > > I suspect the number of bugs this catches will be small, but they'll be > in places where the flow of control is complicated. What we want is to > know that there's no "real" value already present. I'd love it if we > could make the macro declare a new name (yeah, I know, mixing > declarations and code). I don't think I can do a declaration and an expression statement at the same time with different scopes, but that would be kind of cool. We did just move to c11 to gain the in-loop iterator declarations... > Of course, we could also end up with people writing a wrapping macro > that sets the variable to NULL before invoking the underlying macro... I hope it won't come to that! :)
Kees Cook <keescook@chromium.org> writes: > I don't think I can do a declaration and an expression statement at the > same time with different scopes, but that would be kind of cool. We did > just move to c11 to gain the in-loop iterator declarations... Yeah, you'd end up creating a statement-level macro, and I think that would have poor syntax: mem_to_flex_dup(struct something *instance, rc, byte_array, count, GFP_KERNEL); if (rc) return rc; I bet you've already considered the simpler form: struct something *instance = mem_to_flex_dup(byte_array, count, GFP_KERNEL); if (IS_ERR(instance)) return PTR_ERR(instance); This doesn't allow you to require a new name, so you effectively lose the check you're trying to insist upon. Some way to ask the compiler 'is this reference dead?' would be nice -- it knows if a valid pointer was passed to free, or if a variable has not been initialized, after all; we just need that exposed at the source level.
On Thu, 2022-05-05 at 13:08 -0700, Keith Packard wrote: > I bet you've already considered the simpler form: > > struct something *instance = mem_to_flex_dup(byte_array, count, GFP_KERNEL); > if (IS_ERR(instance)) > return PTR_ERR(instance); > Sadly, this doesn't work in any way because mem_to_flex_dup() needs to know at least the type, hence passing 'instance', which is simpler than passing 'struct something'. johannes
On Thu, May 5, 2022 at 2:39 PM Kees Cook <keescook@chromium.org> wrote: > On Wed, May 04, 2022 at 11:14:42PM -0400, Paul Moore wrote: > > On Wed, May 4, 2022 at 7:34 PM Gustavo A. R. Silva > > <gustavoars@kernel.org> wrote: > > > > > > Hi Paul, > > > > > > On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote: > > > > On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > [..] > > > > > > > > +++ b/include/uapi/linux/xfrm.h > > > > > @@ -31,9 +31,9 @@ struct xfrm_id { > > > > > struct xfrm_sec_ctx { > > > > > __u8 ctx_doi; > > > > > __u8 ctx_alg; > > > > > - __u16 ctx_len; > > > > > + __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len); > > > > > __u32 ctx_sid; > > > > > - char ctx_str[0]; > > > > > + __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str); > > > > > }; > > > > > > > > While I like the idea of this in principle, I'd like to hear about the > > > > testing you've done on these patches. A previous flex array > > > > conversion in the audit uapi headers ended up causing a problem with > > > > > > I'm curious about which commit caused those problems...? > > > > Commit ed98ea2128b6 ("audit: replace zero-length array with > > flexible-array member"), however, as I said earlier, the problem was > > actually with SWIG, it just happened to be triggered by the kernel > > commit. There was a brief fedora-devel mail thread about the problem, > > see the link below: > > > > * https://www.spinics.net/lists/fedora-devel/msg297991.html > > Wow, that's pretty weird -- it looks like SWIG was scraping the headers > to build its conversions? I assume SWIG has been fixed now? I honestly don't know, the audit userspace was hacking around it with some header file duplication/munging last I heard, but I try to avoid having to touch Steve's audit userspace code. > > To reiterate, I'm supportive of changes like this, but I would like to > > hear how it was tested to ensure there are no unexpected problems with > > userspace. If there are userspace problems it doesn't mean we can't > > make changes like this, it just means we need to ensure that the > > userspace issues are resolved first. > > Well, as this is the first and only report of any problems with [0] -> [] > conversions (in UAPI or anywhere) that I remember seeing, and they've > been underway since at least v5.9, I hadn't been doing any new testing. ... and for whatever it is worth, I wasn't expecting it to be a problem either. Surprise :) > So, for this case, I guess I should ask what tests you think would be > meaningful here? Anything using #include should be fine: > https://codesearch.debian.net/search?q=linux%2Fxfrm.h&literal=1&perpkg=1 > Which leaves just this, which may be doing something weird: > > libabigail_2.0-1/tests/data/test-diff-filter/test-PR27569-v0.abi > </data-member> > <data-member access="public" layout-offset-in-bits="128"> > <var-decl name="seq_hi" type-id="3f1a6b60" visibility="default" filepath="include/uapi/linux/xfrm.h" line="97" column="1"/> > </data-member> > <data-member access="public" layout-offset-in-bits="160"> > > But I see that SWIG doesn't show up in a search for linux/audit.h: > https://codesearch.debian.net/search?q=linux%2Faudit.h&literal=1&perpkg=1 > > So this may not be a sufficient analysis... I think from a practical perspective ensuring that the major IPsec/IKE tools, e.g. the various *SWANs, that know about labeled IPSec still build and can set/get the SA/SPD labels correctly would be sufficient. I seriously doubt there would be any problems, but who knows.
On Thu, May 05, 2022 at 07:16:18PM -0400, Paul Moore wrote: > On Thu, May 5, 2022 at 2:39 PM Kees Cook <keescook@chromium.org> wrote: > > On Wed, May 04, 2022 at 11:14:42PM -0400, Paul Moore wrote: > > > On Wed, May 4, 2022 at 7:34 PM Gustavo A. R. Silva > > > <gustavoars@kernel.org> wrote: > > > > > > > > Hi Paul, > > > > > > > > On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote: > > > > > On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > > > [..] > > > > > > > > > > +++ b/include/uapi/linux/xfrm.h > > > > > > @@ -31,9 +31,9 @@ struct xfrm_id { > > > > > > struct xfrm_sec_ctx { > > > > > > __u8 ctx_doi; > > > > > > __u8 ctx_alg; > > > > > > - __u16 ctx_len; > > > > > > + __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len); > > > > > > __u32 ctx_sid; > > > > > > - char ctx_str[0]; > > > > > > + __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str); > > > > > > }; > > > > > > > > > > While I like the idea of this in principle, I'd like to hear about the > > > > > testing you've done on these patches. A previous flex array > > > > > conversion in the audit uapi headers ended up causing a problem with > > > > > > > > I'm curious about which commit caused those problems...? > > > > > > Commit ed98ea2128b6 ("audit: replace zero-length array with > > > flexible-array member"), however, as I said earlier, the problem was > > > actually with SWIG, it just happened to be triggered by the kernel > > > commit. There was a brief fedora-devel mail thread about the problem, > > > see the link below: > > > > > > * https://www.spinics.net/lists/fedora-devel/msg297991.html > > > > Wow, that's pretty weird -- it looks like SWIG was scraping the headers > > to build its conversions? I assume SWIG has been fixed now? > > I honestly don't know, the audit userspace was hacking around it with > some header file duplication/munging last I heard, but I try to avoid > having to touch Steve's audit userspace code. > > > > To reiterate, I'm supportive of changes like this, but I would like to > > > hear how it was tested to ensure there are no unexpected problems with > > > userspace. If there are userspace problems it doesn't mean we can't > > > make changes like this, it just means we need to ensure that the > > > userspace issues are resolved first. > > > > Well, as this is the first and only report of any problems with [0] -> [] > > conversions (in UAPI or anywhere) that I remember seeing, and they've > > been underway since at least v5.9, I hadn't been doing any new testing. > > ... and for whatever it is worth, I wasn't expecting it to be a > problem either. Surprise :) > > > So, for this case, I guess I should ask what tests you think would be > > meaningful here? Anything using #include should be fine: > > https://codesearch.debian.net/search?q=linux%2Fxfrm.h&literal=1&perpkg=1 > > Which leaves just this, which may be doing something weird: > > > > libabigail_2.0-1/tests/data/test-diff-filter/test-PR27569-v0.abi > > </data-member> > > <data-member access="public" layout-offset-in-bits="128"> > > <var-decl name="seq_hi" type-id="3f1a6b60" visibility="default" filepath="include/uapi/linux/xfrm.h" line="97" column="1"/> > > </data-member> > > <data-member access="public" layout-offset-in-bits="160"> > > > > But I see that SWIG doesn't show up in a search for linux/audit.h: > > https://codesearch.debian.net/search?q=linux%2Faudit.h&literal=1&perpkg=1 > > > > So this may not be a sufficient analysis... > > I think from a practical perspective ensuring that the major IPsec/IKE > tools, e.g. the various *SWANs, that know about labeled IPSec still > build and can set/get the SA/SPD labels correctly would be sufficient. > I seriously doubt there would be any problems, but who knows. There are certainly some cases in which the transformation of zero-length arrays into flexible-array members can bring some issues to the surface[1][2]. This is the first time that we know of one of them in user-space. However, we haven't transformed the arrays in UAPI yet (with the exception of a couple of cases[3][4]). But that is something that we are planning to try soon[5]. -- Gustavo [1] https://github.com/KSPP/linux/issues?q=invalid+use+of+flexible+array [2] https://github.com/KSPP/linux/issues?q=invalid+application+of+%E2%80%98sizeof%E2%80%99+to+incomplete+type [3] https://git.kernel.org/linus/db243b796439c0caba47865564d8acd18a301d18 [4] https://git.kernel.org/linus/d6cdad870358128c1e753e6258e295ab8a5a2429 [5] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-next/kspp-fam0-uapi
From: Johannes Berg > Sent: 05 May 2022 21:13 > On Thu, 2022-05-05 at 13:08 -0700, Keith Packard wrote: > > > > I bet you've already considered the simpler form: > > > > struct something *instance = mem_to_flex_dup(byte_array, count, GFP_KERNEL); > > if (IS_ERR(instance)) > > return PTR_ERR(instance); > > > > Sadly, this doesn't work in any way because mem_to_flex_dup() needs to > know at least the type, hence passing 'instance', which is simpler than > passing 'struct something'. You can use: struct something *instance; mem_to_flex_dup(instance, byte_array, count, GFP_KERNEL); if (IS_ERR(instance)) return PTR_ERR(instance); and have mem_to_flex_dup() (which must be a #define) update 'instance'. (You can require &instance - and just precede all the uses with an extra '*' to make it more obvious the variable is updated. But there is little point requiring it be NULL.) If you really want to define the variable mid-block you can use: mem_to_flex_dup(struct something *, instance, byte_array, count, GFP_KERNEL); but I really hate having declarations anywhere other than the top of a function because it makes them hard for the 'mk1 eyeball' to spot. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Kees Cook <keescook@chromium.org> wrote: > struct afs_acl { > - u32 size; > - u8 data[]; > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, size); > + DECLARE_FLEX_ARRAY_ELEMENTS(u8, data); > }; Oof... That's really quite unpleasant syntax. Is it not possible to have mem_to_flex_dup() and friends work without that? You are telling them the fields they have to fill in. > + struct afs_acl *acl = NULL; > > - acl = kmalloc(sizeof(*acl) + size, GFP_KERNEL); > - if (!acl) { > + if (mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL)) { Please don't do that. Either do: acl = mem_to_flex_dup(buffer, size, GFP_KERNEL); if (!acl) or: acl = mem_to_flex_dup(buffer, size, GFP_KERNEL); if (IS_ERR(acl)) Please especially don't make it that an apparent 'true' return indicates an error. If you absolutely must return the acl pointer through the argument list (presumably because it's actually a macro), make it return false on failure: if (!mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL)) or return and explicitly check for an error code: if (mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL) < 0) or: ret = mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL); if (ret < 0) (or use != 0 rather than < 0) David
Kees Cook <keescook@chromium.org> wrote: > I'm happy to also point out that the conversions (patches 5+) are actually > a net reduction in lines of code: > 49 files changed, 154 insertions(+), 244 deletions(-) That doesn't mean that it's actually code that's clearer to read. I would say that it's actually less clear. In a bunch of places, you've done something like: - e = kmalloc(...); - if (!e) + if (__mem_to_flex_dup(&e, ...)) The problem is that, to me at least, it looks like: - e = kmalloc(...); - if (kmalloc failed) + if (__mem_to_flex_dup(&e, ...) succeeded) David
On Thu, May 12, 2022 at 10:41:05PM +0100, David Howells wrote: > > Kees Cook <keescook@chromium.org> wrote: > > > struct afs_acl { > > - u32 size; > > - u8 data[]; > > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, size); > > + DECLARE_FLEX_ARRAY_ELEMENTS(u8, data); > > }; > > Oof... That's really quite unpleasant syntax. Is it not possible to have > mem_to_flex_dup() and friends work without that? You are telling them the > fields they have to fill in. Other threads discussed this too. I'm hoping to have something more flexible (pardon the pun) in v2. > [...] > or: > > ret = mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL); > if (ret < 0) > > (or use != 0 rather than < 0) Sure, I can make the tests more explicit. The kerndoc, etc all shows it's using < 0 for errors.
Hi, This is the next phase of memcpy() buffer bounds checking[1], which starts by adding a new set of helpers to address common code patterns that result in memcpy() usage that can't be easily verified by the compiler (i.e. dynamic bounds due to flexible arrays). The runtime WARN from memcpy has been posted before, but now there's more context around alternatives for refactoring false positives, etc. The core of this series is patches 2 (flex_array.h), 3 (flex_array KUnit), and 4 (runtime memcpy WARN). Patch 1 is a fix to land before 4 (and I can send separately), and everything else are examples of what the conversions look like for one of the helpers, mem_to_flex_dup(). These will need to land via their respective trees, but they all depend on patch 2, which I'm hoping to land in the coming merge window. I'm happy to also point out that the conversions (patches 5+) are actually a net reduction in lines of code: 49 files changed, 154 insertions(+), 244 deletions(-) Anyway, please let me know what you think. And apologies in advance if this is spammy; the CC list got rather large due to the "treewide" nature of the example conversions. Also available here: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=flexcpy/next-20220502 -Kees [1] https://lwn.net/Articles/864521/ Kees Cook (32): netlink: Avoid memcpy() across flexible array boundary Introduce flexible array struct memcpy() helpers flex_array: Add Kunit tests fortify: Add run-time WARN for cross-field memcpy() brcmfmac: Use mem_to_flex_dup() with struct brcmf_fweh_queue_item iwlwifi: calib: Prepare to use mem_to_flex_dup() iwlwifi: calib: Use mem_to_flex_dup() with struct iwl_calib_result iwlwifi: mvm: Use mem_to_flex_dup() with struct ieee80211_key_conf p54: Use mem_to_flex_dup() with struct p54_cal_database wcn36xx: Use mem_to_flex_dup() with struct wcn36xx_hal_ind_msg nl80211: Use mem_to_flex_dup() with struct cfg80211_cqm_config cfg80211: Use mem_to_flex_dup() with struct cfg80211_bss_ies mac80211: Use mem_to_flex_dup() with several structs af_unix: Use mem_to_flex_dup() with struct unix_address 802/garp: Use mem_to_flex_dup() with struct garp_attr 802/mrp: Use mem_to_flex_dup() with struct mrp_attr net/flow_offload: Use mem_to_flex_dup() with struct flow_action_cookie firewire: Use __mem_to_flex_dup() with struct iso_interrupt_event afs: Use mem_to_flex_dup() with struct afs_acl ASoC: sigmadsp: Use mem_to_flex_dup() with struct sigmadsp_data soc: qcom: apr: Use mem_to_flex_dup() with struct apr_rx_buf atags_proc: Use mem_to_flex_dup() with struct buffer Bluetooth: Use mem_to_flex_dup() with struct hci_op_configure_data_path IB/hfi1: Use mem_to_flex_dup() for struct tid_rb_node Drivers: hv: utils: Use mem_to_flex_dup() with struct cn_msg ima: Use mem_to_flex_dup() with struct modsig KEYS: Use mem_to_flex_dup() with struct user_key_payload selinux: Use mem_to_flex_dup() with xfrm and sidtab xtensa: Use mem_to_flex_dup() with struct property usb: gadget: f_fs: Use mem_to_flex_dup() with struct ffs_buffer xenbus: Use mem_to_flex_dup() with struct read_buffer esas2r: Use __mem_to_flex() with struct atto_ioctl arch/arm/kernel/atags_proc.c | 12 +- arch/xtensa/platforms/xtfpga/setup.c | 9 +- drivers/firewire/core-cdev.c | 7 +- drivers/hv/hv_utils_transport.c | 7 +- drivers/infiniband/hw/hfi1/user_exp_rcv.c | 7 +- drivers/infiniband/hw/hfi1/user_exp_rcv.h | 4 +- drivers/net/wireless/ath/wcn36xx/smd.c | 8 +- drivers/net/wireless/ath/wcn36xx/smd.h | 4 +- .../broadcom/brcm80211/brcmfmac/fweh.c | 11 +- drivers/net/wireless/intel/iwlwifi/dvm/agn.h | 2 +- .../net/wireless/intel/iwlwifi/dvm/calib.c | 23 +- .../net/wireless/intel/iwlwifi/dvm/ucode.c | 8 +- drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 8 +- drivers/net/wireless/intersil/p54/eeprom.c | 8 +- drivers/net/wireless/intersil/p54/p54.h | 4 +- drivers/scsi/esas2r/atioctl.h | 1 + drivers/scsi/esas2r/esas2r_ioctl.c | 11 +- drivers/soc/qcom/apr.c | 12 +- drivers/usb/gadget/function/f_fs.c | 11 +- drivers/xen/xenbus/xenbus_dev_frontend.c | 12 +- fs/afs/internal.h | 4 +- fs/afs/xattr.c | 7 +- include/keys/user-type.h | 4 +- include/linux/flex_array.h | 637 ++++++++++++++++++ include/linux/fortify-string.h | 70 +- include/linux/of.h | 3 +- include/linux/string.h | 1 + include/net/af_unix.h | 14 +- include/net/bluetooth/hci.h | 4 +- include/net/cfg80211.h | 4 +- include/net/flow_offload.h | 4 +- include/net/garp.h | 4 +- include/net/mac80211.h | 4 +- include/net/mrp.h | 4 +- include/uapi/linux/connector.h | 4 +- include/uapi/linux/firewire-cdev.h | 4 +- include/uapi/linux/netlink.h | 1 + include/uapi/linux/stddef.h | 14 + include/uapi/linux/xfrm.h | 4 +- lib/Kconfig.debug | 12 +- lib/Makefile | 1 + lib/flex_array_kunit.c | 523 ++++++++++++++ net/802/garp.c | 9 +- net/802/mrp.c | 9 +- net/bluetooth/hci_request.c | 9 +- net/core/flow_offload.c | 7 +- net/mac80211/cfg.c | 22 +- net/mac80211/ieee80211_i.h | 12 +- net/netlink/af_netlink.c | 5 +- net/unix/af_unix.c | 7 +- net/wireless/core.h | 4 +- net/wireless/nl80211.c | 15 +- net/wireless/scan.c | 21 +- security/integrity/ima/ima_modsig.c | 12 +- security/keys/user_defined.c | 7 +- security/selinux/ss/sidtab.c | 9 +- security/selinux/xfrm.c | 7 +- sound/soc/codecs/sigmadsp.c | 11 +- 58 files changed, 1409 insertions(+), 253 deletions(-) create mode 100644 include/linux/flex_array.h create mode 100644 lib/flex_array_kunit.c