* [PATCH v2 1/3] ima: Call test's cleanup inside ima_setup.sh cleanup
2019-04-05 16:52 [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs Petr Vorel
@ 2019-04-05 16:52 ` Petr Vorel
2019-04-11 0:59 ` Mimi Zohar
2019-04-05 16:52 ` [PATCH v2 2/3] shell: Add $TST_DEVICE as default parameter to tst_umount Petr Vorel
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Petr Vorel @ 2019-04-05 16:52 UTC (permalink / raw)
To: ltp
Cc: Petr Vorel, Mimi Zohar, Ignaz Forster, Fabian Vogt,
Marcus Meissner, linux-integrity
to work the same way as setup
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
testcases/kernel/security/integrity/ima/tests/ima_setup.sh | 6 +++++-
.../kernel/security/integrity/ima/tests/ima_violations.sh | 2 --
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index 52551190a..cbded42c2 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -20,7 +20,8 @@
TST_TESTFUNC="test"
TST_SETUP_CALLER="$TST_SETUP"
TST_SETUP="ima_setup"
-TST_CLEANUP="${TST_CLEANUP:-ima_cleanup}"
+TST_CLEANUP_CALLER="$TST_CLEANUP"
+TST_CLEANUP="ima_cleanup"
TST_NEEDS_TMPDIR=1
TST_NEEDS_ROOT=1
@@ -95,6 +96,9 @@ ima_setup()
ima_cleanup()
{
local dir
+
+ [ -n "$TST_CLEANUP_CALLER" ] && $TST_CLEANUP_CALLER
+
for dir in $UMOUNT; do
umount $dir
done
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
index 74223c221..a44bd1230 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
@@ -51,8 +51,6 @@ cleanup()
{
[ "$PRINTK_RATE_LIMIT" != "0" ] && \
sysctl -wq kernel.printk_ratelimit=$PRINTK_RATE_LIMIT
-
- ima_cleanup
}
open_file_read()
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/3] ima: Call test's cleanup inside ima_setup.sh cleanup
2019-04-05 16:52 ` [PATCH v2 1/3] ima: Call test's cleanup inside ima_setup.sh cleanup Petr Vorel
@ 2019-04-11 0:59 ` Mimi Zohar
2019-04-11 5:51 ` Petr Vorel
0 siblings, 1 reply; 19+ messages in thread
From: Mimi Zohar @ 2019-04-11 0:59 UTC (permalink / raw)
To: Petr Vorel, ltp
Cc: Mimi Zohar, Ignaz Forster, Fabian Vogt, Marcus Meissner, linux-integrity
On Fri, 2019-04-05 at 18:52 +0200, Petr Vorel wrote:
> to work the same way as setup
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> testcases/kernel/security/integrity/ima/tests/ima_setup.sh | 6 +++++-
> .../kernel/security/integrity/ima/tests/ima_violations.sh | 2 --
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> index 52551190a..cbded42c2 100644
> --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> @@ -20,7 +20,8 @@
> TST_TESTFUNC="test"
> TST_SETUP_CALLER="$TST_SETUP"
> TST_SETUP="ima_setup"
> -TST_CLEANUP="${TST_CLEANUP:-ima_cleanup}"
> +TST_CLEANUP_CALLER="$TST_CLEANUP"
> +TST_CLEANUP="ima_cleanup"
It seems to be working, but defining TST_SETUP and TST_CLEANUP after
defining the respective _CALLER looks strange. The _CALLER's string
must be empty.
> TST_NEEDS_TMPDIR=1
> TST_NEEDS_ROOT=1
>
> @@ -95,6 +96,9 @@ ima_setup()
> ima_cleanup()
> {
> local dir
> +
> + [ -n "$TST_CLEANUP_CALLER" ] && $TST_CLEANUP_CALLER
> +
Is something else setting TST_CLEANUP_CALLER?
> for dir in $UMOUNT; do
> umount $dir
> done
> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> index 74223c221..a44bd1230 100755
> --- a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> @@ -51,8 +51,6 @@ cleanup()
> {
> [ "$PRINTK_RATE_LIMIT" != "0" ] && \
> sysctl -wq kernel.printk_ratelimit=$PRINTK_RATE_LIMIT
> -
> - ima_cleanup
> }
>
> open_file_read()
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/3] ima: Call test's cleanup inside ima_setup.sh cleanup
2019-04-11 0:59 ` Mimi Zohar
@ 2019-04-11 5:51 ` Petr Vorel
2019-04-11 12:22 ` Mimi Zohar
0 siblings, 1 reply; 19+ messages in thread
From: Petr Vorel @ 2019-04-11 5:51 UTC (permalink / raw)
To: Mimi Zohar
Cc: ltp, Mimi Zohar, Ignaz Forster, Fabian Vogt, Marcus Meissner,
linux-integrity
Hi Mimi,
thanks for your comments.
...
> > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > @@ -20,7 +20,8 @@
> > TST_TESTFUNC="test"
> > TST_SETUP_CALLER="$TST_SETUP"
> > TST_SETUP="ima_setup"
> > -TST_CLEANUP="${TST_CLEANUP:-ima_cleanup}"
> > +TST_CLEANUP_CALLER="$TST_CLEANUP"
> > +TST_CLEANUP="ima_cleanup"
> It seems to be working, but defining TST_SETUP and TST_CLEANUP after
> defining the respective _CALLER looks strange. The _CALLER's string
> must be empty.
TST_{SETUP,CALLER}_CALLER takes setup from the test.
It's IMHO cleaner way allowing tests to set their setup/cleanup functions and
not care that there is also some library setup/cleanup (kind of encapsulation).
We already used this for setup, I wanted to have a same approach for both setup
and cleanup. Sure I can instead add ima_setup/ima_cleanup into tests' setup/cleanup
functions, but both solutions are working and I consider encapsulation as a benefit.
The only problematic thing would be if some test needed to run it's custom
cleanup *before* library one while other tests *after*. But that's not a case here.
We also use this approach in tst_net.sh [1].
> > TST_NEEDS_TMPDIR=1
> > TST_NEEDS_ROOT=1
> > @@ -95,6 +96,9 @@ ima_setup()
> > ima_cleanup()
> > {
> > local dir
> > +
> > + [ -n "$TST_CLEANUP_CALLER" ] && $TST_CLEANUP_CALLER
> > +
> Is something else setting TST_CLEANUP_CALLER?
Kind regards,
Petr
[1] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_net.sh#L11
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/3] ima: Call test's cleanup inside ima_setup.sh cleanup
2019-04-11 5:51 ` Petr Vorel
@ 2019-04-11 12:22 ` Mimi Zohar
2019-04-11 20:21 ` Petr Vorel
0 siblings, 1 reply; 19+ messages in thread
From: Mimi Zohar @ 2019-04-11 12:22 UTC (permalink / raw)
To: Petr Vorel
Cc: ltp, Mimi Zohar, Ignaz Forster, Fabian Vogt, Marcus Meissner,
linux-integrity
On Thu, 2019-04-11 at 07:51 +0200, Petr Vorel wrote:
> Hi Mimi,
>
> thanks for your comments.
>
> ...
> > > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > > @@ -20,7 +20,8 @@
> > > TST_TESTFUNC="test"
> > > TST_SETUP_CALLER="$TST_SETUP"
> > > TST_SETUP="ima_setup"
> > > -TST_CLEANUP="${TST_CLEANUP:-ima_cleanup}"
> > > +TST_CLEANUP_CALLER="$TST_CLEANUP"
> > > +TST_CLEANUP="ima_cleanup"
>
> > It seems to be working, but defining TST_SETUP and TST_CLEANUP after
> > defining the respective _CALLER looks strange. The _CALLER's string
> > must be empty.
> TST_{SETUP,CALLER}_CALLER takes setup from the test.
> It's IMHO cleaner way allowing tests to set their setup/cleanup functions and
> not care that there is also some library setup/cleanup (kind of encapsulation).
I'm not questioning the method for initializing this test. I guess
I'm asking why bother to set TST_{SETUP,CLEANUP}_CALLER this way, if
we know that it isn't set. Why not just initialize it as ""?
Mimi
>
> We already used this for setup, I wanted to have a same approach for both setup
> and cleanup. Sure I can instead add ima_setup/ima_cleanup into tests' setup/cleanup
> functions, but both solutions are working and I consider encapsulation as a benefit.
> The only problematic thing would be if some test needed to run it's custom
> cleanup *before* library one while other tests *after*. But that's not a case here.
> We also use this approach in tst_net.sh [1].
>
> > > TST_NEEDS_TMPDIR=1
> > > TST_NEEDS_ROOT=1
>
> > > @@ -95,6 +96,9 @@ ima_setup()
> > > ima_cleanup()
> > > {
> > > local dir
> > > +
> > > + [ -n "$TST_CLEANUP_CALLER" ] && $TST_CLEANUP_CALLER
> > > +
>
> > Is something else setting TST_CLEANUP_CALLER?
>
>
> Kind regards,
> Petr
>
> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_net.sh#L11
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/3] ima: Call test's cleanup inside ima_setup.sh cleanup
2019-04-11 12:22 ` Mimi Zohar
@ 2019-04-11 20:21 ` Petr Vorel
0 siblings, 0 replies; 19+ messages in thread
From: Petr Vorel @ 2019-04-11 20:21 UTC (permalink / raw)
To: Mimi Zohar
Cc: ltp, Mimi Zohar, Ignaz Forster, Fabian Vogt, Marcus Meissner,
linux-integrity
Hi Mimi,
> > > > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > > > @@ -20,7 +20,8 @@
> > > > TST_TESTFUNC="test"
> > > > TST_SETUP_CALLER="$TST_SETUP"
> > > > TST_SETUP="ima_setup"
> > > > -TST_CLEANUP="${TST_CLEANUP:-ima_cleanup}"
> > > > +TST_CLEANUP_CALLER="$TST_CLEANUP"
> > > > +TST_CLEANUP="ima_cleanup"
> > > It seems to be working, but defining TST_SETUP and TST_CLEANUP after
> > > defining the respective _CALLER looks strange. The _CALLER's string
> > > must be empty.
> > TST_{SETUP,CALLER}_CALLER takes setup from the test.
> > It's IMHO cleaner way allowing tests to set their setup/cleanup functions and
> > not care that there is also some library setup/cleanup (kind of encapsulation).
> I'm not questioning the method for initializing this test. I guess
> I'm asking why bother to set TST_{SETUP,CLEANUP}_CALLER this way, if
> we know that it isn't set. Why not just initialize it as ""?
Sorry, I wasn't clear, TST_{SETUP,CLEANUP}_CALLER are set by (some) tests
(as TST_{SETUP,CLEANUP}):
$ git grep TST_SETUP= testcases/kernel/security/integrity/ima/tests/*.sh |grep -v ima_setup.sh
testcases/kernel/security/integrity/ima/tests/evm_overlay.sh:TST_SETUP="setup"
testcases/kernel/security/integrity/ima/tests/ima_measurements.sh:TST_SETUP="setup"
testcases/kernel/security/integrity/ima/tests/ima_policy.sh:TST_SETUP="setup"
testcases/kernel/security/integrity/ima/tests/ima_violations.sh:TST_SETUP="setup"
$ git grep TST_CLEANUP= testcases/kernel/security/integrity/ima/tests/*.sh |grep -v ima_setup.sh
testcases/kernel/security/integrity/ima/tests/evm_overlay.sh:TST_CLEANUP="cleanup"
testcases/kernel/security/integrity/ima/tests/ima_violations.sh:TST_CLEANUP="cleanup"
And this variables are set before loading ima_setup.sh.
So TST_{SETUP,CLEANUP}_CALLER get value from tests (if defined), overwrites it
with it's own function for tst_test.sh, where it calls them (if defined).
Or am I missing something?
> Mimi
Kind regards,
Petr
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/3] shell: Add $TST_DEVICE as default parameter to tst_umount
2019-04-05 16:52 [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs Petr Vorel
2019-04-05 16:52 ` [PATCH v2 1/3] ima: Call test's cleanup inside ima_setup.sh cleanup Petr Vorel
@ 2019-04-05 16:52 ` Petr Vorel
2019-04-05 16:52 ` [PATCH v2 3/3] ima: Add overlay test Petr Vorel
2019-05-14 12:12 ` [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs Petr Vorel
3 siblings, 0 replies; 19+ messages in thread
From: Petr Vorel @ 2019-04-05 16:52 UTC (permalink / raw)
To: ltp
Cc: Petr Vorel, Mimi Zohar, Ignaz Forster, Fabian Vogt,
Marcus Meissner, linux-integrity
+ use it directly as a cleanup function in df01.sh
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
doc/test-writing-guidelines.txt | 4 ++--
testcases/commands/df/df01.sh | 7 +------
testcases/commands/mkfs/mkfs01.sh | 2 +-
testcases/kernel/security/integrity/ima/tests/ima_setup.sh | 2 +-
testcases/lib/tst_test.sh | 2 +-
5 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index f1912dc12..fc64b418b 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -2115,8 +2115,8 @@ The 'tst_mount' mounts '$TST_DEVICE' of '$TST_FS_TYPE' (optional) to
'$TST_MNT_PARAMS'. The '$TST_MNTPOINT' directory is created if it didn't
exist prior to the function call.
-If the path passed to the 'tst_umount' is not mounted (present in '/proc/mounts')
-it's noop.
+If the path passed (optional, defaults to '$TST_DEVICE') to the 'tst_umount' is
+not mounted (present in '/proc/mounts') it's noop.
Otherwise it retries to umount the filesystem a few times on a failure, which
is a workaround since there are a daemons dumb enough to probe all newly
mounted filesystems, which prevents them from umounting shortly after they
diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh
index 9b0be76fe..3876816dc 100755
--- a/testcases/commands/df/df01.sh
+++ b/testcases/commands/df/df01.sh
@@ -18,7 +18,7 @@
TST_CNT=12
TST_SETUP=setup
-TST_CLEANUP=cleanup
+TST_CLEANUP=tst_umount
TST_TESTFUNC=test
TST_OPTS="f:"
TST_USAGE=usage
@@ -54,11 +54,6 @@ setup()
DF_FS_TYPE=$(mount | grep "$TST_DEVICE" | awk '{print $5}')
}
-cleanup()
-{
- tst_umount $TST_DEVICE
-}
-
df_test()
{
local cmd="$1 -P"
diff --git a/testcases/commands/mkfs/mkfs01.sh b/testcases/commands/mkfs/mkfs01.sh
index 88f7f0baa..28af890b3 100755
--- a/testcases/commands/mkfs/mkfs01.sh
+++ b/testcases/commands/mkfs/mkfs01.sh
@@ -71,7 +71,7 @@ mkfs_verify_size()
{
tst_mount
local blocknum=`df -P -B 1k mntpoint | tail -n1 | awk '{print $2}'`
- tst_umount "$TST_DEVICE"
+ tst_umount
if [ $blocknum -gt "$2" ]; then
return 1
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index cbded42c2..da49eb1b2 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -105,7 +105,7 @@ ima_cleanup()
if [ "$TST_NEEDS_DEVICE" = 1 ]; then
cd $TST_TMPDIR
- tst_umount $TST_DEVICE
+ tst_umount
fi
}
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 512732315..740253df1 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -259,7 +259,7 @@ tst_mount()
tst_umount()
{
- local device="$1"
+ local device="${1:-$TST_DEVICE}"
local i=0
if ! grep -q "$device" /proc/mounts; then
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 3/3] ima: Add overlay test
2019-04-05 16:52 [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs Petr Vorel
2019-04-05 16:52 ` [PATCH v2 1/3] ima: Call test's cleanup inside ima_setup.sh cleanup Petr Vorel
2019-04-05 16:52 ` [PATCH v2 2/3] shell: Add $TST_DEVICE as default parameter to tst_umount Petr Vorel
@ 2019-04-05 16:52 ` Petr Vorel
2019-05-14 18:42 ` Ignaz Forster
2019-05-14 12:12 ` [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs Petr Vorel
3 siblings, 1 reply; 19+ messages in thread
From: Petr Vorel @ 2019-04-05 16:52 UTC (permalink / raw)
To: ltp
Cc: Petr Vorel, Mimi Zohar, Ignaz Forster, Fabian Vogt,
Marcus Meissner, linux-integrity
test demonstrate a bug on overlayfs on current mainline kernel when
combining IMA with EVM.
Based on reproducer made by Ignaz Forster <iforster@suse.de>
used for not upstreamed patchset [1] and previous report [2].
IMA only behavior has already been fixed [3].
NOTE: backup variables are needed because ima_setup.sh calling
tst_mount as well when TMPDIR is on tmpfs device.
[1] https://www.spinics.net/lists/linux-integrity/msg05926.html
[2] https://www.spinics.net/lists/linux-integrity/msg03593.html
[3] https://patchwork.kernel.org/patch/10776231/
Tested-by: Ignaz Forster <iforster@suse.de>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Changes v1->v2:
* Added 2 more tests (suggested by Ignaz): append file and create a new
file. Tests has been further split to 4 tests (previously was only 1).
* Fixed testing on tmpfs (reported by Mimi) by adding TST_NEEDS_DEVICE=1.
* Renamed file to evm_overlay.sh (from ima_overlay.sh; suggested by Ignaz).
Obviously it's about IMA + EVM, but IMHO 'ima' can be left from filename.
* Changed pattern for kernel parameters check (suggested by Mimi).
TODO/questions:
Still not sure what is a proper setup.
Should I check EVM enabled?
/sys/kernel/security/evm should be 1?
I suppose different behaviour between ima_policy=appraise_tcb and
ima_appraise_tcb is just my wrong setup.
Thanks a lot for your comments.
Kind regards,
Petr
---
runtest/ima | 1 +
.../integrity/ima/tests/evm_overlay.sh | 86 +++++++++++++++++++
.../security/integrity/ima/tests/ima_setup.sh | 4 +-
3 files changed, 89 insertions(+), 2 deletions(-)
create mode 100755 testcases/kernel/security/integrity/ima/tests/evm_overlay.sh
diff --git a/runtest/ima b/runtest/ima
index bcae16bb7..f3ea88cf0 100644
--- a/runtest/ima
+++ b/runtest/ima
@@ -3,3 +3,4 @@ ima_measurements ima_measurements.sh
ima_policy ima_policy.sh
ima_tpm ima_tpm.sh
ima_violations ima_violations.sh
+evm_overlay evm_overlay.sh
diff --git a/testcases/kernel/security/integrity/ima/tests/evm_overlay.sh b/testcases/kernel/security/integrity/ima/tests/evm_overlay.sh
new file mode 100755
index 000000000..08ec1ea37
--- /dev/null
+++ b/testcases/kernel/security/integrity/ima/tests/evm_overlay.sh
@@ -0,0 +1,86 @@
+#!/bin/sh
+# Copyright (c) 2019 Petr Vorel <pvorel@suse.cz>
+# Based on reproducer and further discussion with Ignaz Forster <iforster@suse.de>
+
+TST_SETUP="setup"
+TST_CLEANUP="cleanup"
+TST_NEEDS_DEVICE=1
+TST_CNT=4
+. ima_setup.sh
+
+setup()
+{
+ lower="$TST_MNTPOINT/lower"
+ upper="$TST_MNTPOINT/upper"
+ work="$TST_MNTPOINT/work"
+ merged="$TST_MNTPOINT/merged"
+ mkdir -p $lower $upper $work $merged
+
+ device_backup="$TST_DEVICE"
+ TST_DEVICE="overlay"
+
+ fs_type_backup="$TST_FS_TYPE"
+ TST_FS_TYPE="overlay"
+
+ mntpoint_backup="$TST_MNTPOINT"
+ TST_MNTPOINT="$merged"
+
+ params_backup="$TST_MNT_PARAMS"
+ TST_MNT_PARAMS="-o lowerdir=$lower,upperdir=$upper,workdir=$work"
+
+ grep -q -e ima_policy= -e ima_appraise_tcb /proc/cmdline || \
+ tst_brk TCONF "Test requires specify IMA policy as kernel parameter"
+
+ tst_mount
+ mounted=1
+}
+
+test1()
+{
+ local file="foo1.txt"
+
+ tst_res TINFO "overwrite file in overlay"
+ ROD echo lower \> $lower/$file
+ EXPECT_PASS echo overlay \> $merged/$file
+}
+
+test2()
+{
+ local file="foo2.txt"
+
+ tst_res TINFO "append file in overlay"
+ ROD echo lower \> $lower/$file
+ EXPECT_PASS echo overlay \>\> $merged/$file
+}
+
+test3()
+{
+ local file="foo3.txt"
+
+ tst_res TINFO "create a new file in overlay"
+ EXPECT_PASS echo overlay \> $merged/$file
+}
+
+test4()
+{
+ local f
+
+ tst_res TINFO "read all created files"
+ for f in $(find $TST_MNTPOINT -type f); do
+ EXPECT_PASS cat $f \> /dev/null 2\> /dev/null
+ done
+}
+
+cleanup()
+{
+ [ -n "$mounted" ] || return 0
+
+ tst_umount $TST_DEVICE
+
+ TST_DEVICE="$device_backup"
+ TST_FS_TYPE="$fs_type_backup"
+ TST_MNTPOINT="$mntpoint_backup"
+ TST_MNT_PARAMS="$params_backup"
+}
+
+tst_run
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
index da49eb1b2..08aa5300a 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
@@ -64,14 +64,14 @@ print_ima_config()
local config="/boot/config-$(uname -r)"
local i
- tst_res TINFO "/proc/cmdline: $(cat /proc/cmdline)"
-
if [ -r "$config" ]; then
tst_res TINFO "IMA kernel config:"
for i in $(grep ^CONFIG_IMA $config); do
tst_res TINFO "$i"
done
fi
+
+ tst_res TINFO "/proc/cmdline: $(cat /proc/cmdline)"
}
ima_setup()
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/3] ima: Add overlay test
2019-04-05 16:52 ` [PATCH v2 3/3] ima: Add overlay test Petr Vorel
@ 2019-05-14 18:42 ` Ignaz Forster
2019-05-15 11:32 ` Petr Vorel
0 siblings, 1 reply; 19+ messages in thread
From: Ignaz Forster @ 2019-05-14 18:42 UTC (permalink / raw)
To: Petr Vorel, ltp; +Cc: Mimi Zohar, Fabian Vogt, Marcus Meissner, linux-integrity
Hi Petr,
thanks a lot for your continued work on the IMA / EVM tests and sorry
for missing feedback - the mail got lost in my stack of TODO items.
Am 05.04.19 um 18:52 Uhr schrieb Petr Vorel:
> Should I check EVM enabled?
As these tests require an appropriately prepared machine anyway: How
about printing a message whether only IMA or both IMA and EVM are
enabled. These tests make sense in both cases, so I wouldn't block them
on either setup.
> /sys/kernel/security/evm should be 1?
Yes, that should be enough to detect whether EVM is enabled.
> +test1()
> +{
> + local file="foo1.txt"
> +
> + tst_res TINFO "overwrite file in overlay"
> + ROD echo lower \> $lower/$file
> + EXPECT_PASS echo overlay \> $merged/$file
It seems the redirection / escaping is wrong here: the string "overlay"
never ends up in the target file.
> +}
> +
> +test2()
> +{
> + local file="foo2.txt"
> +
> + tst_res TINFO "append file in overlay"
> + ROD echo lower \> $lower/$file
> + EXPECT_PASS echo overlay \>\> $merged/$file
Same here: "overlay" never ends up in the target file.
Ignaz
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/3] ima: Add overlay test
2019-05-14 18:42 ` Ignaz Forster
@ 2019-05-15 11:32 ` Petr Vorel
0 siblings, 0 replies; 19+ messages in thread
From: Petr Vorel @ 2019-05-15 11:32 UTC (permalink / raw)
To: Ignaz Forster
Cc: ltp, Mimi Zohar, Fabian Vogt, Marcus Meissner, linux-integrity
Hi Ignaz,
> thanks a lot for your continued work on the IMA / EVM tests and sorry for
> missing feedback - the mail got lost in my stack of TODO items.
Not a big deal and thanks a lot for a feedback and info.
> Am 05.04.19 um 18:52 Uhr schrieb Petr Vorel:
> > Should I check EVM enabled?
> As these tests require an appropriately prepared machine anyway: How about
> printing a message whether only IMA or both IMA and EVM are enabled. These
> tests make sense in both cases, so I wouldn't block them on either setup.
> > /sys/kernel/security/evm should be 1?
> Yes, that should be enough to detect whether EVM is enabled.
> > +test1()
> > +{
> > + local file="foo1.txt"
> > +
> > + tst_res TINFO "overwrite file in overlay"
> > + ROD echo lower \> $lower/$file
> > + EXPECT_PASS echo overlay \> $merged/$file
> It seems the redirection / escaping is wrong here: the string "overlay"
> never ends up in the target file.
I'm pretty sure that '>' should be escaped for both ROD and EXPECT_PASS.
EXPECT_PASS should fail (something like "evm_overlay 1 TFAIL: echo overlay ...
failed unexpectedly") if "overlay" string didn't get into $merged/$file file.
Can you sent whole output?
Kind regards,
Petr
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs
2019-04-05 16:52 [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs Petr Vorel
` (2 preceding siblings ...)
2019-04-05 16:52 ` [PATCH v2 3/3] ima: Add overlay test Petr Vorel
@ 2019-05-14 12:12 ` Petr Vorel
2019-05-14 19:19 ` Ignaz Forster
2019-05-15 3:01 ` Mimi Zohar
3 siblings, 2 replies; 19+ messages in thread
From: Petr Vorel @ 2019-05-14 12:12 UTC (permalink / raw)
To: Mimi Zohar, Ignaz Forster
Cc: Fabian Vogt, Marcus Meissner, linux-integrity, ltp
Hi Mimi, Ignaz,
Mimi, could you please have a second look on this [4] patchset? We've had a
discussion about second patch [5], I can drop it if you don't like it, but
that's not a main concern about this test. More important is whether the
testcase looks valid for you. It's about overlayfs broken in IMA+EVM,
which is currently broken on mainline.
There is different reproducer (C code) for a slightly different scenario,
but I'm not going to port it before this got merged.
Ignaz, could you please test this patchset? Could you, please, share your setup?
ima_policy=appraise_tcb kernel parameter and loading IMA and EVM keys over
dracut-ima scripts? (IMA appraisal and EVM using digital signatures? I guess
using hashes for IMA appraisal would work as well).
Kind regards,
Petr
> this is a second version of patch demonstrating a bug on overlayfs when
> combining IMA with EVM. There is ongoing work made by Ignaz Forster and
> Fabian Vogt [1] [2], IMA only behavior was already fixed [3].
> Main patch is the last one (previous are just a cleanup and not changed).
> [1] https://www.spinics.net/lists/linux-integrity/msg05926.html
> [2] https://www.spinics.net/lists/linux-integrity/msg03593.html
> [3] https://patchwork.kernel.org/patch/10776231/
[4] https://patchwork.ozlabs.org/project/ltp/list/?series=101213&state=*
[5] https://patchwork.ozlabs.org/patch/1078553/
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs
2019-05-14 12:12 ` [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs Petr Vorel
@ 2019-05-14 19:19 ` Ignaz Forster
2019-05-15 11:34 ` Petr Vorel
2019-05-15 3:01 ` Mimi Zohar
1 sibling, 1 reply; 19+ messages in thread
From: Ignaz Forster @ 2019-05-14 19:19 UTC (permalink / raw)
To: Petr Vorel; +Cc: Mimi Zohar, Fabian Vogt, Marcus Meissner, linux-integrity, ltp
[-- Attachment #1: Type: text/plain, Size: 830 bytes --]
Hi Petr,
Am 14.05.19 um 14:12 Uhr schrieb Petr Vorel:
> Could you, please, share your setup?
The system was installed with IMA and EVM enabled during installation,
using the following kernel parameters:
"ima_policy=appraise_tcb ima_appraise=fix evm=fix"
The EVM key was generated in the live system before starting the actual
installation and copied into the installed system later.
See the attached installation notes for an openSUSE system (which should
also be usable on other distributions).
> ima_policy=appraise_tcb kernel parameter and loading IMA and EVM keys over
> dracut-ima scripts?
Exactly.
> (IMA appraisal and EVM using digital signatures? I guess
> using hashes for IMA appraisal would work as well).
I focused on hashes, as those are more relevant for the overlayfs use
case I was thinking of.
Ignaz
[-- Attachment #2: IMA_EVM.txt --]
[-- Type: text/plain, Size: 1393 bytes --]
Manual IMA / EVM installation:
* Use a net install image (some of the necessary packages are not available in DVD image)
* Boot install system with "ima_policy=appraise_tcb ima_appraise=fix evm=fix" (for IMA measurement, IMA appraisal and EVM protection)
* Proceed with installation until summary screen, but do not start the installation yet
* Remove "evm=fix" from kernel boot parameters
* Change kernel boot parameter "ima_appraise=fix" to "ima_appraise=appraise_tcb"
* Select package "dracut-ima" (required for early boot EVM support) for installation
* Change to a console window
* mkdir /etc/keys
* /bin/keyctl add user kmk-user "`dd if=/dev/urandom bs=1 count=32 2>/dev/null`" @u
* /bin/keyctl pipe `/bin/keyctl search @u user kmk-user` > /etc/keys/kmk-user.blob
* /bin/keyctl add encrypted evm-key "new user:kmk-user 64" @u
* /bin/keyctl pipe `/bin/keyctl search @u encrypted evm-key` >/etc/keys/evm.blob
* cat <<END >/etc/sysconfig/masterkey
MASTERKEYTYPE="user"
MASTERKEY="/etc/keys/kmk-user.blob"
END
* cat <<END >/etc/sysconfig/evm
EVMKEY="/etc/keys/evm.blob"
END
* mount -t securityfs security /sys/kernel/security
* echo 1 >/sys/kernel/security/evm
* Go back to the installation summary screen and start the installation
* During the installation execute the following commands from the console:
* cp -r /etc/keys /mnt/etc/
* cp /etc/sysconfig/{evm,masterkey} /mnt/etc/sysconfig/
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs
2019-05-14 12:12 ` [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs Petr Vorel
2019-05-14 19:19 ` Ignaz Forster
@ 2019-05-15 3:01 ` Mimi Zohar
2019-05-15 12:08 ` Petr Vorel
1 sibling, 1 reply; 19+ messages in thread
From: Mimi Zohar @ 2019-05-15 3:01 UTC (permalink / raw)
To: Petr Vorel, Mimi Zohar, Ignaz Forster
Cc: Fabian Vogt, Marcus Meissner, linux-integrity, ltp
On Tue, 2019-05-14 at 14:12 +0200, Petr Vorel wrote:
> Hi Mimi, Ignaz,
>
> Mimi, could you please have a second look on this [4] patchset? We've had a
> discussion about second patch [5], I can drop it if you don't like it, but
> that's not a main concern about this test. More important is whether the
> testcase looks valid for you. It's about overlayfs broken in IMA+EVM,
> which is currently broken on mainline.
The first two patches are fine. From the test, I'm seeing the
following results:
evm_overlay 1 TINFO: overwrite file in overlay
tst_rod: Failed to open '(null)' for writing: Operation not permitted
evm_overlay 1 TFAIL: echo overlay > mntpoint/merged/foo1.txt failed unexpectedly
evm_overlay 2 TINFO: append file in overlay: mntpoint/lower/foo2.txt
evm_overlay 2 TPASS: echo overlay >> mntpoint/merged/foo2.txt passed as expected
evm_overlay 3 TINFO: create a new file in overlay
evm_overlay 3 TPASS: echo overlay > mntpoint/merged/foo3.txt passed as expected
evm_overlay 4 TINFO: read all created files
evm_overlay 4 TFAIL: cat mntpoint/merged/foo1.txt > /dev/null 2> /dev/null failed unexpectedly
evm_overlay 4 TFAIL: cat mntpoint/merged/foo2.txt > /dev/null 2> /dev/null failed unexpectedly
evm_overlay 4 TFAIL: cat mntpoint/merged/foo3.txt > /dev/null 2> /dev/null failed unexpectedly
evm_overlay 5 TINFO: SELinux enabled in enforcing mode, this may affect test results
evm_overlay 5 TINFO: You can try to disable it with TST_DISABLE_SELINUX=1 (requires super/root)
evm_overlay 5 TINFO: loaded SELinux profiles: none
With "evm: instead of using the overlayfs i_ino, use the real i_ino"
patch, I'm only seeing the first failure.
Mimi
> There is different reproducer (C code) for a slightly different scenario,
> but I'm not going to port it before this got merged.
>
> Ignaz, could you please test this patchset? Could you, please, share your setup?
> ima_policy=appraise_tcb kernel parameter and loading IMA and EVM keys over
> dracut-ima scripts? (IMA appraisal and EVM using digital signatures? I guess
> using hashes for IMA appraisal would work as well).
>
> Kind regards,
> Petr
>
> > this is a second version of patch demonstrating a bug on overlayfs when
> > combining IMA with EVM. There is ongoing work made by Ignaz Forster and
> > Fabian Vogt [1] [2], IMA only behavior was already fixed [3].
>
> > Main patch is the last one (previous are just a cleanup and not changed).
>
> > [1] https://www.spinics.net/lists/linux-integrity/msg05926.html
> > [2] https://www.spinics.net/lists/linux-integrity/msg03593.html
> > [3] https://patchwork.kernel.org/patch/10776231/
>
> [4] https://patchwork.ozlabs.org/project/ltp/list/?series=101213&state=*
> [5] https://patchwork.ozlabs.org/patch/1078553/
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs
2019-05-15 3:01 ` Mimi Zohar
@ 2019-05-15 12:08 ` Petr Vorel
2019-05-16 22:10 ` Mimi Zohar
0 siblings, 1 reply; 19+ messages in thread
From: Petr Vorel @ 2019-05-15 12:08 UTC (permalink / raw)
To: Mimi Zohar
Cc: Ignaz Forster, Fabian Vogt, Marcus Meissner, linux-integrity, ltp
Hi Mimi,
> The first two patches are fine. From the test, I'm seeing the
> following results:
Thanks a lot for reviewing and testing.
> evm_overlay 1 TINFO: overwrite file in overlay
> tst_rod: Failed to open '(null)' for writing: Operation not permitted
> evm_overlay 1 TFAIL: echo overlay > mntpoint/merged/foo1.txt failed unexpectedly
I've fixed '(null)' [1], with that one applied it should be 'mntpoint/merged/foo1.txt'
But what is strange to me is that it continues to execute second line. return 1 [2]
should cause ROD() to quit with TBROK [3].
Maybe that ROD in test1() should be replaced EXPECT_PASS.
> evm_overlay 2 TINFO: append file in overlay: mntpoint/lower/foo2.txt
> evm_overlay 2 TPASS: echo overlay >> mntpoint/merged/foo2.txt passed as expected
> evm_overlay 3 TINFO: create a new file in overlay
> evm_overlay 3 TPASS: echo overlay > mntpoint/merged/foo3.txt passed as expected
> evm_overlay 4 TINFO: read all created files
> evm_overlay 4 TFAIL: cat mntpoint/merged/foo1.txt > /dev/null 2> /dev/null failed unexpectedly
> evm_overlay 4 TFAIL: cat mntpoint/merged/foo2.txt > /dev/null 2> /dev/null failed unexpectedly
> evm_overlay 4 TFAIL: cat mntpoint/merged/foo3.txt > /dev/null 2> /dev/null failed unexpectedly
> evm_overlay 5 TINFO: SELinux enabled in enforcing mode, this may affect test results
> evm_overlay 5 TINFO: You can try to disable it with TST_DISABLE_SELINUX=1 (requires super/root)
> evm_overlay 5 TINFO: loaded SELinux profiles: none
> With "evm: instead of using the overlayfs i_ino, use the real i_ino"
> patch, I'm only seeing the first failure.
> Mimi
Kind regards,
Petr
[1] https://github.com/linux-test-project/ltp/commit/8a35daf6bb175391fd43cd28d9ca2d0d5b06157c
[2] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_rod.c#L117
[3] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_test.sh#L150
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs
2019-05-15 12:08 ` Petr Vorel
@ 2019-05-16 22:10 ` Mimi Zohar
2019-05-17 7:50 ` Petr Vorel
0 siblings, 1 reply; 19+ messages in thread
From: Mimi Zohar @ 2019-05-16 22:10 UTC (permalink / raw)
To: Petr Vorel
Cc: Ignaz Forster, Fabian Vogt, Marcus Meissner, linux-integrity, ltp
Hi Petr,
On Wed, 2019-05-15 at 14:08 +0200, Petr Vorel wrote:
> > evm_overlay 1 TINFO: overwrite file in overlay
> > tst_rod: Failed to open '(null)' for writing: Operation not permitted
> > evm_overlay 1 TFAIL: echo overlay > mntpoint/merged/foo1.txt failed unexpectedly
> I've fixed '(null)' [1], with that one applied it should be 'mntpoint/merged/foo1.txt'
Thanks!
> But what is strange to me is that it continues to execute second line. return 1 [2]
> should cause ROD() to quit with TBROK [3].
> Maybe that ROD in test1() should be replaced EXPECT_PASS.
With just the first patch of Ignaz's path set [1] and the TPM 2.0 test
[2], there aren't any errors. Without [1], it's now failing with the
correct name. I'm now seeing:
evm_overlay 1 TINFO: $TMPDIR is on tmpfs => run on loop device
evm_overlay 1 TINFO: Formatting /dev/loop0 with ext3 extra opts=''
evm_overlay 1 TINFO: overwrite file in overlay
tst_rod: Failed to open 'mntpoint/merged/foo1.txt' for writing: Permission denied
evm_overlay 1 TFAIL: echo overlay > mntpoint/merged/foo1.txt failed unexpectedly
Mimi
[1] https://www.spinics.net/lists/linux-integrity/msg05926.html
[2] https://lore.kernel.org/linux-integrity/1558041162.3971.2.camel@linux.ibm.com/T/#u
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs
2019-05-16 22:10 ` Mimi Zohar
@ 2019-05-17 7:50 ` Petr Vorel
2019-05-17 11:00 ` Mimi Zohar
0 siblings, 1 reply; 19+ messages in thread
From: Petr Vorel @ 2019-05-17 7:50 UTC (permalink / raw)
To: Mimi Zohar
Cc: Ignaz Forster, Fabian Vogt, Marcus Meissner, linux-integrity, ltp
Hi Mimi,
> > But what is strange to me is that it continues to execute second line. return 1 [2]
> > should cause ROD() to quit with TBROK [3].
> > Maybe that ROD in test1() should be replaced EXPECT_PASS.
> With just the first patch of Ignaz's path set [1] and the TPM 2.0 test
> [2], there aren't any errors. Without [1], it's now failing with the
> correct name. I'm now seeing:
I guess, that justifies [1] to be merged into kernel.
> evm_overlay 1 TINFO: $TMPDIR is on tmpfs => run on loop device
> evm_overlay 1 TINFO: Formatting /dev/loop0 with ext3 extra opts=''
> evm_overlay 1 TINFO: overwrite file in overlay
> tst_rod: Failed to open 'mntpoint/merged/foo1.txt' for writing: Permission denied
> evm_overlay 1 TFAIL: echo overlay > mntpoint/merged/foo1.txt failed unexpectedly
That still does not explain, why test doesn't exit before this last line.
I'll have a closer look into it. But as I wrote, I'll make these changes:
diff --git testcases/kernel/security/integrity/ima/tests/evm_overlay.sh testcases/kernel/security/integrity/ima/tests/evm_overlay.sh
index 08ec1ea37..1d05b9e1c 100755
--- testcases/kernel/security/integrity/ima/tests/evm_overlay.sh
+++ testcases/kernel/security/integrity/ima/tests/evm_overlay.sh
@@ -40,7 +40,7 @@ test1()
local file="foo1.txt"
tst_res TINFO "overwrite file in overlay"
- ROD echo lower \> $lower/$file
+ EXPECT_PASS echo lower \> $lower/$file
EXPECT_PASS echo overlay \> $merged/$file
}
@@ -49,7 +49,7 @@ test2()
local file="foo2.txt"
tst_res TINFO "append file in overlay"
- ROD echo lower \> $lower/$file
+ EXPECT_PASS echo lower \> $lower/$file
EXPECT_PASS echo overlay \>\> $merged/$file
}
---
If it's ok for you and it's a valid test do you give an ack?
Kind regards,
Petr
> Mimi
> [1] https://www.spinics.net/lists/linux-integrity/msg05926.html
> [2] https://lore.kernel.org/linux-integrity/1558041162.3971.2.camel@linux.ibm.com/T/#u
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs
2019-05-17 7:50 ` Petr Vorel
@ 2019-05-17 11:00 ` Mimi Zohar
2019-05-17 15:41 ` Petr Vorel
0 siblings, 1 reply; 19+ messages in thread
From: Mimi Zohar @ 2019-05-17 11:00 UTC (permalink / raw)
To: Petr Vorel
Cc: Ignaz Forster, Fabian Vogt, Marcus Meissner, linux-integrity, ltp
On Fri, 2019-05-17 at 09:50 +0200, Petr Vorel wrote:
>
> If it's ok for you and it's a valid test do you give an ack?
Of course! Thanks!
Mimi
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] LTP reproducer on broken IMA on overlayfs
2019-05-17 11:00 ` Mimi Zohar
@ 2019-05-17 15:41 ` Petr Vorel
0 siblings, 0 replies; 19+ messages in thread
From: Petr Vorel @ 2019-05-17 15:41 UTC (permalink / raw)
To: Mimi Zohar
Cc: Ignaz Forster, Fabian Vogt, Marcus Meissner, linux-integrity, ltp
Hi Mimi,
> On Fri, 2019-05-17 at 09:50 +0200, Petr Vorel wrote:
> > If it's ok for you and it's a valid test do you give an ack?
> Of course! Thanks!
Thanks! I'll add also Ignaz's description (create README.md in IMA folder),
thus probably send a v3 to ML first, but not expecting to get much review :).
> Mimi
Kind regards,
Petr
^ permalink raw reply [flat|nested] 19+ messages in thread