[1/1] IMA/ima_keys.sh Fix policy content check usage

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(-)

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