On Fri, Aug 09, 2019 at 01:20:45PM +0100, Cristian Marussi wrote:

Hi

On 8/9/19 12:16 PM, Dave Martin wrote:

quoted

On Fri, Aug 09, 2019 at 11:54:06AM +0100, Cristian Marussi wrote:

quoted

Hi

On 8/2/19 6:02 PM, Cristian Marussi wrote:

quoted

Added some arm64/signal specific boilerplate and utility code to help
further testcase development.

A simple testcase and related helpers are also introduced in this commit:
mangle_pstate_invalid_compat_toggle is a simple mangle testcase which
messes with the ucontext_t from within the sig_handler, trying to toggle
PSTATE state bits to switch the system between 32bit/64bit execution state.
Expects SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---

[...]

quoted

quoted

diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c
index 31788a1d33a4..c0f3cd1b560a 100644
--- a/tools/testing/selftests/arm64/signal/test_signals_utils.c
+++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c

@@ -23,21 +23,25 @@ extern struct tdescr *current;
 static int sig_copyctx = SIGTRAP;
 static char *feats_store[FMAX_END] = {
-       "SSBS",
-       "PAN",
-       "UAO",
+       " SSBS ",
+       " PAN ",
+       " UAO ",
 };
 #define MAX_FEATS_SZ   128
+static char feats_string[MAX_FEATS_SZ];
+
 static inline char *feats_to_string(unsigned long feats)
 {
-       static char feats_string[MAX_FEATS_SZ];
+       for (int i = 0; i < FMAX_END; i++) {
+               size_t tlen = 0;
-       for (int i = 0; i < FMAX_END && feats_store[i][0]; i++) {
-               if (feats & 1UL << i)
-                       snprintf(feats_string, MAX_FEATS_SZ - 1, "%s %s ",
-                                feats_string, feats_store[i]);
+               if (feats & 1UL << i) {
+                       strncat(feats_string, feats_store[i],

Should this be feats_string + tlen?

strncat appends to the end of a NULL terminated string overwriting the NULL itself and
appending its own NULL (as long as dest and src do not overlap and fits the max size param),
so it must be fed the start of the dest string to which we are appending

quoted

quoted

+                               MAX_FEATS_SZ - 1 - tlen);

I see.  Yes, you're right -- I was confusing strncat() with strncpy().

quoted

An assert(tlen <= MAX_FEATS_SZ - 1) is probably a good idea here,
in case more features are added to feats_store[] someday.

Yes in fact...if not it would be simply truncated silently

I think MAX_FEATS - 1 - tlen would overflow.  tlen is a size_t, so the
result would might be a giant unsigned number in this case, leading to a
potential buffer overrun in strncat().

quoted

quoted

+                       tlen += strlen(feats_store[i]);
+               }

Don't we need to initialise tlen outside the loop?  Otherwise we just
zero it again after the +=.

..and that's a bug :<

OK

quoted

quoted

        }
        return feats_string;

@@ -190,7 +194,7 @@ static void default_handler(int signum, siginfo_t *si, void *uc)
                /* it's a bug in the test code when this assert fail */
                assert(!current->sig_trig || current->triggered);
                fprintf(stderr,
-                       "SIG_OK -- SP:%p  si_addr@:0x%p  si_code:%d  token@:0x%p  offset:%ld\n",
+                       "SIG_OK -- SP:%llX  si_addr@:0x%p  si_code:%d  token@:0x%p  offset:%ld\n",

For consistency, can we have a "0x" prefix?

I think %p usually generates a "0x" prefix by itself, so 0x%p might give
a double prefix.

Yes you are right.

Moreover I'm in doubt what to do generally with all these stderr
output, because I optionally discard to null testing standalone, but
this is not what the KSFT framework runner script does, so
arm64/signal tests end up being overly verbose when run from the
framework (even if tests use anyway the KSFT exit codes conventions
so all the results are correctly reported); but I suppose I'll
receive a clear indication on this matter from the maintainers at the
end...

Sure, keep the prints for now.  If they're potentially useful we can
always find a way to make them optional.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel