diff mbox series

lib: stackinit: Adjust target string to 8 bytes for m68k

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

Commit Message

Kees Cook Feb. 27, 2024, 10:46 p.m. UTC
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(-)

Comments

Andreas Schwab Feb. 27, 2024, 10:56 p.m. UTC | #1
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.
Guenter Roeck Feb. 27, 2024, 11:03 p.m. UTC | #2
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
Geert Uytterhoeven Feb. 28, 2024, 7:58 a.m. UTC | #3
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 mbox series

Patch

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 -		\