| Message ID | 20240227224653.work.603-kees@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | c2efa5387c2676815ebbb6a954bf72fef2609709 |
| Headers | show |
| Series | lib: stackinit: Adjust target string to 8 bytes for m68k | expand |
On Feb 27 2024, Kees Cook wrote: > For reasons I cannot understand, m68k moves the start of the stack frame > for consecutive calls to the same function It's called optimisation.
On Tue, Feb 27, 2024 at 02:46:56PM -0800, Kees Cook wrote: > For reasons I cannot understand, m68k moves the start of the stack frame > for consecutive calls to the same function if the function's test > variable is larger than 8 bytes. This was only happening for the char > array test (obviously), so adjust the length of the string for m68k > only. I want the array size to be longer than "unsigned long" for every > given architecture, so the other remain unchanged. > > Additionally adjust the error message to be a bit more clear about > what's happened, and move the KUNIT check outside of the consecutive > calls to minimize what happens between them. > > Reported-by: Guenter Roeck <linux@roeck-us.net> > Closes: https://lore.kernel.org/lkml/a0d10d50-2720-4ecd-a2c6-c2c5e5aeee65@roeck-us.net/ > Signed-off-by: Kees Cook <keescook@chromium.org> Hmm, guess I misunderstood the other e-mail. Anyway, it works. After re-enabling the m68k tests: Tested-by: Guenter Roeck <linux@roeck-us.net> I'll also run it through qemu for other architectures to be sure that there is no negative impact. Thanks, Guenter
Hi Kees, On Tue, Feb 27, 2024 at 11:47 PM Kees Cook <keescook@chromium.org> wrote: > For reasons I cannot understand, m68k moves the start of the stack frame > for consecutive calls to the same function if the function's test > variable is larger than 8 bytes. This was only happening for the char > array test (obviously), so adjust the length of the string for m68k > only. I want the array size to be longer than "unsigned long" for every > given architecture, so the other remain unchanged. > > Additionally adjust the error message to be a bit more clear about > what's happened, and move the KUNIT check outside of the consecutive > calls to minimize what happens between them. Thanks for your patch! > Reported-by: Guenter Roeck <linux@roeck-us.net> > Closes: https://lore.kernel.org/lkml/a0d10d50-2720-4ecd-a2c6-c2c5e5aeee65@roeck-us.net/ Do reports have an expiration date? ;-) Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Closes: https://lore.kernel.org/r/CAMuHMdX_g1tbiUL9PUQdqaegrEzCNN3GtbSvSBFYAL4TzvstFg@mail.gmail.com Closes: https://lore.kernel.org/r/CAMuHMdW6N40+0gGQ+LSrN64Mo4A0-ELAm0pR3gWQ0mNanyBuUQ@mail.gmail.com > Signed-off-by: Kees Cook <keescook@chromium.org> Finally all parts of this test are passing on m68k, great! Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> Gr{oetje,eeting}s, Geert
diff --git a/lib/stackinit_kunit.c b/lib/stackinit_kunit.c index 05947a2feb93..dc3c68f46f0a 100644 --- a/lib/stackinit_kunit.c +++ b/lib/stackinit_kunit.c @@ -63,7 +63,16 @@ static bool stackinit_range_contains(char *haystack_start, size_t haystack_size, #define FETCH_ARG_STRING(var) var #define FETCH_ARG_STRUCT(var) &var +/* + * On m68k, if the leaf function test variable is longer than 8 bytes, + * the start of the stack frame moves. 8 is sufficiently large to + * test m68k char arrays, but leave it at 16 for other architectures. + */ +#ifdef CONFIG_M68K +#define FILL_SIZE_STRING 8 +#else #define FILL_SIZE_STRING 16 +#endif #define INIT_CLONE_SCALAR /**/ #define INIT_CLONE_STRING [FILL_SIZE_STRING] @@ -165,19 +174,23 @@ static noinline void test_ ## name (struct kunit *test) \ /* Verify all bytes overwritten with 0xFF. */ \ for (sum = 0, i = 0; i < target_size; i++) \ sum += (check_buf[i] != 0xFF); \ - KUNIT_ASSERT_EQ_MSG(test, sum, 0, \ - "leaf fill was not 0xFF!?\n"); \ /* Clear entire check buffer for later bit tests. */ \ memset(check_buf, 0x00, sizeof(check_buf)); \ /* Extract stack-defined variable contents. */ \ ignored = leaf_ ##name((unsigned long)&ignored, 0, \ FETCH_ARG_ ## which(zero)); \ + /* \ + * Delay the sum test to here to do as little as \ + * possible between the two leaf function calls. \ + */ \ + KUNIT_ASSERT_EQ_MSG(test, sum, 0, \ + "leaf fill was not 0xFF!?\n"); \ \ /* Validate that compiler lined up fill and target. */ \ KUNIT_ASSERT_TRUE_MSG(test, \ stackinit_range_contains(fill_start, fill_size, \ target_start, target_size), \ - "stack fill missed target!? " \ + "stackframe was not the same between calls!? " \ "(fill %zu wide, target offset by %d)\n", \ fill_size, \ (int)((ssize_t)(uintptr_t)fill_start - \
For reasons I cannot understand, m68k moves the start of the stack frame for consecutive calls to the same function if the function's test variable is larger than 8 bytes. This was only happening for the char array test (obviously), so adjust the length of the string for m68k only. I want the array size to be longer than "unsigned long" for every given architecture, so the other remain unchanged. Additionally adjust the error message to be a bit more clear about what's happened, and move the KUNIT check outside of the consecutive calls to minimize what happens between them. Reported-by: Guenter Roeck <linux@roeck-us.net> Closes: https://lore.kernel.org/lkml/a0d10d50-2720-4ecd-a2c6-c2c5e5aeee65@roeck-us.net/ Signed-off-by: Kees Cook <keescook@chromium.org> --- Cc: Geert Uytterhoeven <geert@linux-m68k.org> --- lib/stackinit_kunit.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)