| Message ID | 20200807112929.8984-1-pvorel@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] IMA/ima_keys.sh Fix policy content check usage | expand |
Hi all, ... > --- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh > +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh > @@ -16,11 +16,14 @@ TST_NEEDS_DEVICE=1 > # (450d0fd51564 - "IMA: Call workqueue functions to measure queued keys") > test1() > { > - local keyrings keycheck_lines keycheck_line templates test_file="file.txt" > + local keyrings keycheck_lines keycheck_line templates > + local policy="func=KEY_CHECK" > + local test_file="file.txt" > tst_res TINFO "verifying key measurement for keyrings and templates specified in IMA policy file" > - keycheck_lines=$(require_ima_policy_content "func=KEY_CHECK" "") > + require_ima_policy_content $policy > + keycheck_lines=$(check_ima_policy_content $policy "") > keycheck_line=$(echo "$keycheck_lines" | grep "keyrings" | head -n1) While working on this patchset, I wonder, why we don't check for 'func=KEY_CHECK.*keyrings' in single grep call instead of grepping it twice. IMHO single grep call is enough. Or am I missing something? Kind regards, Petr
On 8/7/20 7:15 AM, Petr Vorel wrote: > Hi all, > > ... >> --- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh >> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh >> @@ -16,11 +16,14 @@ TST_NEEDS_DEVICE=1 >> # (450d0fd51564 - "IMA: Call workqueue functions to measure queued keys") >> test1() >> { >> - local keyrings keycheck_lines keycheck_line templates test_file="file.txt" >> + local keyrings keycheck_lines keycheck_line templates >> + local policy="func=KEY_CHECK" >> + local test_file="file.txt" > >> tst_res TINFO "verifying key measurement for keyrings and templates specified in IMA policy file" > >> - keycheck_lines=$(require_ima_policy_content "func=KEY_CHECK" "") >> + require_ima_policy_content $policy >> + keycheck_lines=$(check_ima_policy_content $policy "") >> keycheck_line=$(echo "$keycheck_lines" | grep "keyrings" | head -n1) > While working on this patchset, I wonder, why we don't check for > 'func=KEY_CHECK.*keyrings' in single grep call instead of grepping it twice. > IMHO single grep call is enough. Or am I missing something? > Instead require_ima_policy_content calling "tst_brk" in error condition, it can just return false and the caller can handle it as appropriate. Would that avoid two grep calls? thanks, -lakshmi
Hi all, > ... > > --- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh > > +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh > > @@ -16,11 +16,14 @@ TST_NEEDS_DEVICE=1 > > # (450d0fd51564 - "IMA: Call workqueue functions to measure queued keys") > > test1() > > { > > - local keyrings keycheck_lines keycheck_line templates test_file="file.txt" > > + local keyrings keycheck_lines keycheck_line templates > > + local policy="func=KEY_CHECK" > > + local test_file="file.txt" > > tst_res TINFO "verifying key measurement for keyrings and templates specified in IMA policy file" > > - keycheck_lines=$(require_ima_policy_content "func=KEY_CHECK" "") > > + require_ima_policy_content $policy > > + keycheck_lines=$(check_ima_policy_content $policy "") > > keycheck_line=$(echo "$keycheck_lines" | grep "keyrings" | head -n1) > While working on this patchset, I wonder, why we don't check for > 'func=KEY_CHECK.*keyrings' in single grep call instead of grepping it twice. > IMHO single grep call is enough. Or am I missing something? OK, the order can be different as (according to doc [1] as Mimi remarked in some older mail) only action is fixed on first place, order of conditions isn't defined. Thus this would make it: grep -E '^measure.*(func=KEY_CHECK.*keyrings|keyrings.*func=KEY_CHECK)' But both tests have the requirement in common only 'func=KEY_CHECK', thus I'll do some preparations for next test. (+ we didn't require measure, thus dont_measure could fit into previous check as well). Kind regards, Petr [1] https://www.kernel.org/doc/Documentation/ABI/testing/ima_policy
Hi, FYI: this patch was resent in [1] as part of [2] [1] https://patchwork.ozlabs.org/project/ltp/patch/20200807204652.5928-2-pvorel@suse.cz/ [2] https://patchwork.ozlabs.org/project/ltp/list/?series=194714 Kind regards, Petr
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh index 3aea26056..b5c5c0542 100755 --- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh @@ -16,11 +16,14 @@ TST_NEEDS_DEVICE=1 # (450d0fd51564 - "IMA: Call workqueue functions to measure queued keys") test1() { - local keyrings keycheck_lines keycheck_line templates test_file="file.txt" + local keyrings keycheck_lines keycheck_line templates + local policy="func=KEY_CHECK" + local test_file="file.txt" tst_res TINFO "verifying key measurement for keyrings and templates specified in IMA policy file" - keycheck_lines=$(require_ima_policy_content "func=KEY_CHECK" "") + require_ima_policy_content $policy + keycheck_lines=$(check_ima_policy_content $policy "") keycheck_line=$(echo "$keycheck_lines" | grep "keyrings" | head -n1) if [ -z "$keycheck_line" ]; then
require_ima_policy_content cannot be used in subshell $() evaluation, because tst_brk does not quit the test. It calls cleanup for the subshell process and main process then continue: ima_keys 1 TCONF: IMA policy does not specify 'func=KEY_CHECK' => Here it's running first cleanup. umount errors are because parent shell process still has $PWD in directory to be unmounted: umount: /tmp/LTP_ima_keys.0dIVrwJKIG/mntpoint: target is busy. ima_keys 1 TINFO: umount(/dev/loop0) failed, try 1 ... ima_keys 1 TINFO: Likely gvfsd-trash is probing newly mounted fs, kill it to speed up tests. umount: /tmp/LTP_ima_keys.0dIVrwJKIG/mntpoint: target is busy. ... ima_keys 1 TINFO: umount(/dev/loop0) failed, try 50 ... ima_keys 1 TINFO: Likely gvfsd-trash is probing newly mounted fs, kill it to speed up tests. ima_keys 1 TWARN: Failed to umount(/dev/loop0) after 50 retries tst_device.c:222: WARN: ioctl(/dev/loop0, LOOP_CLR_FD, 0) no ENXIO for too long Usage: tst_device acquire [size [filename]] or: tst_device release /path/to/device ima_keys 1 TWARN: Failed to release device '/dev/loop0' rm: cannot remove '/tmp/LTP_ima_keys.0dIVrwJKIG/mntpoint': Device or resource busy ima_keys 1 TINFO: AppArmor enabled, this may affect test results ima_keys 1 TINFO: it can be disabled with TST_DISABLE_APPARMOR=1 (requires super/root) ima_keys 1 TINFO: loaded AppArmor profiles: none /opt/ltp/testcases/bin/ima_keys.sh: line 25: 6166 Terminated sleep $sec && tst_res TBROK "test killed, timeout! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9 -$pid (wd: ~) => Here it should quit after running cleanup, but instead continue running: ima_keys 1 TCONF: ima policy does not specify a keyrings to check Fixes: f20f44d72 ("IMA/ima_keys.sh: Fix policy readability check") Signed-off-by: Petr Vorel <pvorel@suse.cz> --- Hi, do I miss some obvious way how to fix either the test, require_ima_policy_content or LTP shell API to be able to run just require_ima_policy_content in the previous form? (i.e. using subshell assigment) Kind regards, Petr testcases/kernel/security/integrity/ima/tests/ima_keys.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)