From: Dave Martin <Dave.Martin@arm.com> To: Cristian Marussi <cristian.marussi@arm.com> Cc: linux-arm-kernel@lists.infradead.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH v3 02/11] kselftest: arm64: adds first test and common utils Date: Fri, 9 Aug 2019 13:32:34 +0100 [thread overview] Message-ID: <20190809123233.GM10425@arm.com> (raw) In-Reply-To: <4a73fcdf-911e-b44a-ce6b-f9bcde34eec8@arm.com> On Fri, Aug 09, 2019 at 01:20:45PM +0100, Cristian Marussi wrote: > > Hi > > On 8/9/19 12:16 PM, Dave Martin wrote: > >On Fri, Aug 09, 2019 at 11:54:06AM +0100, Cristian Marussi wrote: > >>Hi > >> > >>On 8/2/19 6:02 PM, Cristian Marussi wrote: > >>>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> > >>>--- [...] > >>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 > > >>+ MAX_FEATS_SZ - 1 - tlen); I see. Yes, you're right -- I was confusing strncat() with strncpy(). > >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(). > > >>+ 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 > > > >> } > >> 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
WARNING: multiple messages have this Message-ID (diff)
From: Dave Martin <Dave.Martin@arm.com> To: Cristian Marussi <cristian.marussi@arm.com> Cc: linux-kselftest@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 02/11] kselftest: arm64: adds first test and common utils Date: Fri, 9 Aug 2019 13:32:34 +0100 [thread overview] Message-ID: <20190809123233.GM10425@arm.com> (raw) In-Reply-To: <4a73fcdf-911e-b44a-ce6b-f9bcde34eec8@arm.com> On Fri, Aug 09, 2019 at 01:20:45PM +0100, Cristian Marussi wrote: > > Hi > > On 8/9/19 12:16 PM, Dave Martin wrote: > >On Fri, Aug 09, 2019 at 11:54:06AM +0100, Cristian Marussi wrote: > >>Hi > >> > >>On 8/2/19 6:02 PM, Cristian Marussi wrote: > >>>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> > >>>--- [...] > >>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 > > >>+ MAX_FEATS_SZ - 1 - tlen); I see. Yes, you're right -- I was confusing strncat() with strncpy(). > >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(). > > >>+ 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 > > > >> } > >> 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
next prev parent reply other threads:[~2019-08-09 12:32 UTC|newest] Thread overview: 112+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-02 17:02 [PATCH v3 00/11] Add arm64/signal initial kselftest support Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-02 17:02 ` [PATCH v3 01/11] kselftest: arm64: introduce new boilerplate code Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-13 16:23 ` Dave Martin 2019-08-13 16:23 ` Dave Martin 2019-08-27 12:14 ` Cristian Marussi 2019-08-27 12:14 ` Cristian Marussi 2019-08-02 17:02 ` [PATCH v3 02/11] kselftest: arm64: adds first test and common utils Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-06 15:50 ` Cristian Marussi 2019-08-06 15:50 ` Cristian Marussi 2019-08-07 15:42 ` Cristian Marussi 2019-08-07 15:42 ` Cristian Marussi 2019-08-09 10:54 ` Cristian Marussi 2019-08-09 10:54 ` Cristian Marussi 2019-08-09 11:16 ` Dave Martin 2019-08-09 11:16 ` Dave Martin 2019-08-09 12:20 ` Cristian Marussi 2019-08-09 12:20 ` Cristian Marussi 2019-08-09 12:32 ` Dave Martin [this message] 2019-08-09 12:32 ` Dave Martin 2019-08-12 12:43 ` Amit Kachhap 2019-08-12 12:43 ` Amit Kachhap 2019-08-13 13:22 ` Cristian Marussi 2019-08-13 13:22 ` Cristian Marussi 2019-08-14 10:22 ` Amit Kachhap 2019-08-14 10:22 ` Amit Kachhap 2019-08-27 14:24 ` Cristian Marussi 2019-08-27 14:24 ` Cristian Marussi 2019-08-13 16:24 ` Dave Martin 2019-08-13 16:24 ` Dave Martin 2019-08-28 17:34 ` Cristian Marussi 2019-08-28 17:34 ` Cristian Marussi 2019-09-03 15:34 ` Dave Martin 2019-09-03 15:34 ` Dave Martin 2019-09-03 16:08 ` Cristian Marussi 2019-09-03 16:08 ` Cristian Marussi 2019-08-02 17:02 ` [PATCH v3 03/11] kselftest: arm64: mangle_pstate_invalid_daif_bits Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-13 16:24 ` Dave Martin 2019-08-13 16:24 ` Dave Martin 2019-08-29 10:19 ` Cristian Marussi 2019-08-29 10:19 ` Cristian Marussi 2019-08-02 17:02 ` [PATCH v3 04/11] kselftest: arm64: mangle_pstate_invalid_mode_el Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-13 16:24 ` Dave Martin 2019-08-13 16:24 ` Dave Martin 2019-08-29 11:50 ` Cristian Marussi 2019-08-29 11:50 ` Cristian Marussi 2019-08-02 17:02 ` [PATCH v3 05/11] kselftest: arm64: mangle_pstate_ssbs_regs Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-13 16:25 ` Dave Martin 2019-08-13 16:25 ` Dave Martin 2019-08-29 15:35 ` Cristian Marussi 2019-08-29 15:35 ` Cristian Marussi 2019-08-02 17:02 ` [PATCH v3 06/11] kselftest: arm64: fake_sigreturn_bad_magic Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-13 16:25 ` Dave Martin 2019-08-13 16:25 ` Dave Martin 2019-08-30 14:29 ` Cristian Marussi 2019-08-30 14:29 ` Cristian Marussi 2019-09-04 10:05 ` Dave Martin 2019-09-04 10:05 ` Dave Martin 2019-09-04 10:37 ` Cristian Marussi 2019-09-04 10:37 ` Cristian Marussi 2019-09-04 10:47 ` Dave Martin 2019-09-04 10:47 ` Dave Martin 2019-08-02 17:02 ` [PATCH v3 07/11] kselftest: arm64: fake_sigreturn_bad_size_for_magic0 Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-13 16:25 ` Dave Martin 2019-08-13 16:25 ` Dave Martin 2019-08-30 14:49 ` Cristian Marussi 2019-08-30 14:49 ` Cristian Marussi 2019-08-02 17:02 ` [PATCH v3 08/11] kselftest: arm64: fake_sigreturn_missing_fpsimd Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-13 16:26 ` Dave Martin 2019-08-13 16:26 ` Dave Martin 2019-08-30 14:55 ` Cristian Marussi 2019-08-30 14:55 ` Cristian Marussi 2019-08-02 17:02 ` [PATCH v3 09/11] kselftest: arm64: fake_sigreturn_duplicated_fpsimd Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-13 16:26 ` Dave Martin 2019-08-13 16:26 ` Dave Martin 2019-08-30 15:11 ` Cristian Marussi 2019-08-30 15:11 ` Cristian Marussi 2019-08-02 17:02 ` [PATCH v3 10/11] kselftest: arm64: fake_sigreturn_bad_size Cristian Marussi 2019-08-02 17:02 ` Cristian Marussi 2019-08-13 16:26 ` Dave Martin 2019-08-13 16:26 ` Dave Martin 2019-08-30 15:21 ` Cristian Marussi 2019-08-30 15:21 ` Cristian Marussi 2019-08-02 17:03 ` [PATCH v3 11/11] kselftest: arm64: fake_sigreturn_misaligned_sp Cristian Marussi 2019-08-02 17:03 ` Cristian Marussi 2019-08-07 16:04 ` Cristian Marussi 2019-08-07 16:04 ` Cristian Marussi 2019-08-13 16:28 ` Dave Martin 2019-08-13 16:28 ` Dave Martin 2019-08-30 15:22 ` Cristian Marussi 2019-08-30 15:22 ` Cristian Marussi 2019-08-13 16:27 ` Dave Martin 2019-08-13 16:27 ` Dave Martin 2019-08-30 16:33 ` Cristian Marussi 2019-08-30 16:33 ` Cristian Marussi 2019-08-13 16:22 ` [PATCH v3 00/11] Add arm64/signal initial kselftest support Dave Martin 2019-08-13 16:22 ` Dave Martin 2019-08-30 16:40 ` Cristian Marussi 2019-08-30 16:40 ` Cristian Marussi 2019-09-02 10:53 ` Dave Martin 2019-09-02 10:53 ` Dave Martin 2019-09-02 11:30 ` Cristian Marussi 2019-09-02 11:30 ` Cristian Marussi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190809123233.GM10425@arm.com \ --to=dave.martin@arm.com \ --cc=cristian.marussi@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kselftest@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.