[PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL

[PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL

[Kees Cook]

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

Re: [PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL

[Guillaume Tucker]

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-20210623.0/x86_64/x86_64_defconfig+x86-chromebook+kselftest/clang-13/lab-collabora/kselftest-lkdtm-asus-C523NA-A20057-coral.html


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
> 

RE: [PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL

[David Laight]

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)

Re: [selftests/lkdtm] 84d8cf25b0: kernel_BUG_at_drivers/misc/lkdtm/bugs.c

[Kees Cook]

On Wed, Jun 23, 2021 at 10:35:50PM +0800, kernel test robot wrote:
> 
> 
> Greeting,
> 
> FYI, we noticed the following commit (built with gcc-9):
> 
> commit: 84d8cf25b0f80da0ac229214864654a7662ec7e4 ("[PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL")
> url: https://github.com/0day-ci/linux/commits/Kees-Cook/selftests-lkdtm-Use-bin-sh-not-SHELL/20210619-105959
> base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git next
> 
> in testcase: kernel-selftests
> version: kernel-selftests-x86_64-f8879e85-1_20210618
> with following parameters:
> 
> 	group: lkdtm
> 	ucode: 0xe2

Heh. Yes, this is working as intended. :) Most of the lkdtm tests will
trigger Oopses, and this is by design: it is checking that the kernel
catches bad conditions and freaks out appropriately.

-Kees

-- 
Kees Cook

Re: [PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL

[Kees Cook]

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-20210623.0/x86_64/x86_64_defconfig+x86-chromebook+kselftest/clang-13/lab-collabora/kselftest-lkdtm-asus-C523NA-A20057-coral.html

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

-- 
Kees Cook

Re: [PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL

[Guillaume Tucker]

+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-20210623.0/x86_64/x86_64_defconfig+x86-chromebook+kselftest/clang-13/lab-collabora/kselftest-lkdtm-asus-C523NA-A20057-coral.html
> 
> 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

Re: [PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL

[Kees Cook]

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

-- 
Kees Cook

RE: [PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL

[David Laight]

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)

Re: [PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL

[Kees Cook]

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. :)

-- 
Kees Cook

RE: [PATCH v2] selftests/lkdtm: Use /bin/sh not $SHELL

[David Laight]

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)