Some environments do not set $SHELL when running tests. There's no need to use $SHELL here anyway, so just replace it with hard-coded path instead. Additionally avoid using bash-isms in the command, so that regular /bin/sh can be used.
Suggested-by: Guillaume Tucker guillaume.tucker@collabora.com Fixes: 46d1a0f03d66 ("selftests/lkdtm: Add tests for LKDTM targets") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook keescook@chromium.org --- tools/testing/selftests/lkdtm/run.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index bb7a1775307b..0f9f22ac004b 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh @@ -78,8 +78,9 @@ dmesg > "$DMESG"
# Most shells yell about signals and we're expecting the "cat" process # to usually be killed by the kernel. So we have to run it in a sub-shell -# and silence errors. -($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true +# to avoid terminating this script. Leave stderr alone, just in case +# something _else_ happens. +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true
# Record and dump the results dmesg | comm --nocheck-order -13 "$DMESG" - > "$LOG" || true
On 19/06/2021 03:58, Kees Cook wrote:
Some environments do not set $SHELL when running tests. There's no need to use $SHELL here anyway, so just replace it with hard-coded path instead. Additionally avoid using bash-isms in the command, so that regular /bin/sh can be used.
Suggested-by: Guillaume Tucker guillaume.tucker@collabora.com Fixes: 46d1a0f03d66 ("selftests/lkdtm: Add tests for LKDTM targets") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook keescook@chromium.org
Tested-by: "kernelci.org bot" bot@kernelci.org
Sample staging results with this patch applied on top of next-20210622:
https://staging.kernelci.org/test/plan/id/60d2dbdc3cfb88da0924bf41/
Full log:
https://storage.staging.kernelci.org/kernelci/staging-next/staging-next-2021...
This was tested using Debian Buster with the default shell being "dash", which doesn't support Bash-specific features.
Thanks, Guillaume
tools/testing/selftests/lkdtm/run.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index bb7a1775307b..0f9f22ac004b 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh @@ -78,8 +78,9 @@ dmesg > "$DMESG" # Most shells yell about signals and we're expecting the "cat" process # to usually be killed by the kernel. So we have to run it in a sub-shell -# and silence errors. -($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true +# to avoid terminating this script. Leave stderr alone, just in case +# something _else_ happens. +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true # Record and dump the results dmesg | comm --nocheck-order -13 "$DMESG" - > "$LOG" || true
From: Guillaume Tucker
Sent: 23 June 2021 13:40
...
diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index bb7a1775307b..0f9f22ac004b 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh @@ -78,8 +78,9 @@ dmesg > "$DMESG"
# Most shells yell about signals and we're expecting the "cat" process # to usually be killed by the kernel. So we have to run it in a sub-shell -# and silence errors. -($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true +# to avoid terminating this script. Leave stderr alone, just in case +# something _else_ happens. +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true
I was having trouble parsing that command - and I'm good at shell scripts. I think the extra subshell the 'echo' is in doesn't help. In fact, is either subshell needed? Surely: /bin/sh -c "echo '$test' | cat >$trigger" || true will work just as well?
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Jun 23, 2021 at 01:43:04PM +0000, David Laight wrote:
From: Guillaume Tucker
Sent: 23 June 2021 13:40
...
diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index bb7a1775307b..0f9f22ac004b 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh @@ -78,8 +78,9 @@ dmesg > "$DMESG"
# Most shells yell about signals and we're expecting the "cat" process # to usually be killed by the kernel. So we have to run it in a sub-shell -# and silence errors. -($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true +# to avoid terminating this script. Leave stderr alone, just in case +# something _else_ happens. +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true
I was having trouble parsing that command - and I'm good at shell scripts. I think the extra subshell the 'echo' is in doesn't help. In fact, is either subshell needed? Surely: /bin/sh -c "echo '$test' | cat >$trigger" || true will work just as well?
Ah yeah, and I just tested it to double check, it can be even simpler:
echo "$test" | /bin/sh -c "cat >$TRIGGER" || true
I'll adjust and resend...
-Kees
From: Kees Cook
Sent: 23 June 2021 17:19
On Wed, Jun 23, 2021 at 01:43:04PM +0000, David Laight wrote:
From: Guillaume Tucker
Sent: 23 June 2021 13:40
...
diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index bb7a1775307b..0f9f22ac004b 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh @@ -78,8 +78,9 @@ dmesg > "$DMESG"
# Most shells yell about signals and we're expecting the "cat" process # to usually be killed by the kernel. So we have to run it in a sub-shell -# and silence errors. -($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true +# to avoid terminating this script. Leave stderr alone, just in case +# something _else_ happens. +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true
I was having trouble parsing that command - and I'm good at shell scripts. I think the extra subshell the 'echo' is in doesn't help. In fact, is either subshell needed? Surely: /bin/sh -c "echo '$test' | cat >$trigger" || true will work just as well?
Ah yeah, and I just tested it to double check, it can be even simpler:
echo "$test" | /bin/sh -c "cat >$TRIGGER" || true
You can probably even do:
echo "$test" | /bin/sh -c cat >$TRIGGER || true
(moving the redirect to the outer shell).
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Jun 23, 2021 at 04:27:47PM +0000, David Laight wrote:
From: Kees Cook
Sent: 23 June 2021 17:19
On Wed, Jun 23, 2021 at 01:43:04PM +0000, David Laight wrote:
From: Guillaume Tucker
Sent: 23 June 2021 13:40
...
diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index bb7a1775307b..0f9f22ac004b 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh @@ -78,8 +78,9 @@ dmesg > "$DMESG"
# Most shells yell about signals and we're expecting the "cat" process # to usually be killed by the kernel. So we have to run it in a sub-shell -# and silence errors. -($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true +# to avoid terminating this script. Leave stderr alone, just in case +# something _else_ happens. +(/bin/sh -c '(echo '"$test"') | cat >'"$TRIGGER") || true
I was having trouble parsing that command - and I'm good at shell scripts. I think the extra subshell the 'echo' is in doesn't help. In fact, is either subshell needed? Surely: /bin/sh -c "echo '$test' | cat >$trigger" || true will work just as well?
Ah yeah, and I just tested it to double check, it can be even simpler:
echo "$test" | /bin/sh -c "cat >$TRIGGER" || true
You can probably even do:
echo "$test" | /bin/sh -c cat >$TRIGGER || true
(moving the redirect to the outer shell).
Actually, it looks like the "write" is already happening in the exec'd process, so this can just be:
echo "$test" | cat >$TRIGGER || true
But it still can't be:
echo "$test" >$TRIGGER
which is what I had over-engineered a solution to. :)
From: Kees Cook
Sent: 23 June 2021 23:47
On Wed, Jun 23, 2021 at 04:27:47PM +0000, David Laight wrote:
...
You can probably even do:
echo "$test" | /bin/sh -c cat >$TRIGGER || true
(moving the redirect to the outer shell).
Actually, it looks like the "write" is already happening in the exec'd process, so this can just be:
echo "$test" | cat >$TRIGGER || true
But it still can't be:
echo "$test" >$TRIGGER
which is what I had over-engineered a solution to. :)
That one fails because echo is the shell builtin. But: /bin/echo "$test" >$TRIGGER should be fine.
Quite where the original came from I not sure I want to find out.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Jun 23, 2021 at 01:39:57PM +0100, Guillaume Tucker wrote:
On 19/06/2021 03:58, Kees Cook wrote:
Some environments do not set $SHELL when running tests. There's no need to use $SHELL here anyway, so just replace it with hard-coded path instead. Additionally avoid using bash-isms in the command, so that regular /bin/sh can be used.
Suggested-by: Guillaume Tucker guillaume.tucker@collabora.com Fixes: 46d1a0f03d66 ("selftests/lkdtm: Add tests for LKDTM targets") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook keescook@chromium.org
Tested-by: "kernelci.org bot" bot@kernelci.org
Sample staging results with this patch applied on top of next-20210622:
https://staging.kernelci.org/test/plan/id/60d2dbdc3cfb88da0924bf41/
Full log:
https://storage.staging.kernelci.org/kernelci/staging-next/staging-next-2021...
Awesome! This looks great. :)
What's needed to build these kernels will different CONFIGs? I see a bunch of things (commonly found in distro kernels) that are not set:
CONFIG_SLAB_FREELIST_HARDENED=y CONFIG_FORTIFY_SOURCE=y CONFIG_HARDENED_USERCOPY=y # CONFIG_HARDENED_USERCOPY_FALLBACK is not set
Should I add these to the kselftest "config" file for LKDTM?
Thanks again for the help with this!
-Kees
+kernelci +collabora
On 23/06/2021 15:38, Kees Cook wrote:
On Wed, Jun 23, 2021 at 01:39:57PM +0100, Guillaume Tucker wrote:
On 19/06/2021 03:58, Kees Cook wrote:
Some environments do not set $SHELL when running tests. There's no need to use $SHELL here anyway, so just replace it with hard-coded path instead. Additionally avoid using bash-isms in the command, so that regular /bin/sh can be used.
Suggested-by: Guillaume Tucker guillaume.tucker@collabora.com Fixes: 46d1a0f03d66 ("selftests/lkdtm: Add tests for LKDTM targets") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook keescook@chromium.org
Tested-by: "kernelci.org bot" bot@kernelci.org
Sample staging results with this patch applied on top of next-20210622:
https://staging.kernelci.org/test/plan/id/60d2dbdc3cfb88da0924bf41/
Full log:
https://storage.staging.kernelci.org/kernelci/staging-next/staging-next-2021...
Awesome! This looks great. :)
What's needed to build these kernels will different CONFIGs? I see a bunch of things (commonly found in distro kernels) that are not set:
CONFIG_SLAB_FREELIST_HARDENED=y CONFIG_FORTIFY_SOURCE=y CONFIG_HARDENED_USERCOPY=y # CONFIG_HARDENED_USERCOPY_FALLBACK is not set
Should I add these to the kselftest "config" file for LKDTM?
Yes, that's the current way to do it.
KernelCI is simply concatenating all the config files found under tools/testing/selftests into one big kselftest fragment which is then merged with the defconfig. We could enable arbitrary things for KernelCI but of course it's much better to not do that and stick to what's in the kernel tree.
If you do send such a patch, please CC kernelci@groups.io or myself and we can give it a spin on staging.kernelci.org as well.
Best wishes, Guillaume
linux-kselftest-mirror@lists.linaro.org
-
David Laight -
Guillaume Tucker -
Kees Cook