[PATCH 1/2] IMA: Add a test to verify measurment of keys

[PATCH 1/2] IMA: Add a test to verify measurment of keys

[Lachlan Sneff]

Add a testcase that verifies that the IMA subsystem has correctly
measured keys added to keyrings specified in the IMA policy file.

Additionally, add support for handling a new IMA template descriptor,
namely ima-buf[1], in the IMA measurement tests.

[1]: https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use

Signed-off-by: Lachlan Sneff <t-josne@linux.microsoft.com>
---
 runtest/ima                                   |  1 +
 .../integrity/ima/tests/compute_digest.sh     | 38 ++++++++++
 .../security/integrity/ima/tests/ima_keys.sh  | 72 +++++++++++++++++++
 .../integrity/ima/tests/ima_measurements.sh   | 37 +---------
 4 files changed, 113 insertions(+), 35 deletions(-)
 create mode 100644 testcases/kernel/security/integrity/ima/tests/compute_digest.sh
 create mode 100644 testcases/kernel/security/integrity/ima/tests/ima_keys.sh

diff --git a/runtest/ima b/runtest/ima
index f3ea88cf0..939fb40f0 100644
--- a/runtest/ima
+++ b/runtest/ima
@@ -4,3 +4,4 @@ ima_policy ima_policy.sh
 ima_tpm ima_tpm.sh
 ima_violations ima_violations.sh
 evm_overlay evm_overlay.sh
+ima_keys ima_keys.sh
diff --git a/testcases/kernel/security/integrity/ima/tests/compute_digest.sh b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
new file mode 100644
index 000000000..85f6bf3da
--- /dev/null
+++ b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2009 IBM Corporation
+# Copyright (c) 2018-2020 Petr Vorel <pvorel@suse.cz>
+# Author: Mimi Zohar <zohar@linux.ibm.com>
+
+# TODO: find support for rmd128 rmd256 rmd320 wp256 wp384 tgr128 tgr160
+compute_digest()
+{
+	local algorithm="$1"
+	local file="$2"
+	local digest
+
+	digest="$(${algorithm}sum $file 2>/dev/null | cut -f1 -d ' ')"
+	if [ -n "$digest" ]; then
+		echo "$digest"
+		return 0
+	fi
+
+	digest="$(openssl $algorithm $file 2>/dev/null | cut -f2 -d ' ')"
+	if [ -n "$digest" ]; then
+		echo "$digest"
+		return 0
+	fi
+
+	# uncommon ciphers
+	local arg="$algorithm"
+	case "$algorithm" in
+	tgr192) arg="tiger" ;;
+	wp512) arg="whirlpool" ;;
+	esac
+
+	digest="$(rdigest --$arg $file 2>/dev/null | cut -f1 -d ' ')"
+	if [ -n "$digest" ]; then
+		echo "$digest"
+		return 0
+	fi
+	return 1
+}
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
new file mode 100644
index 000000000..1b0dd0aed
--- /dev/null
+++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2020 Microsoft Corporation
+# Author: Lachlan Sneff <t-josne@linux.microsoft.com>
+#
+# Verify that keys are measured correctly based on policy.
+
+TST_NEEDS_CMDS="awk cut"
+TST_SETUP="setup"
+TST_CNT=1
+TST_NEEDS_DEVICE=1
+
+. ima_setup.sh
+. compute_digest.sh
+
+setup()
+{
+    TEST_FILE="$PWD/test.txt"
+}
+
+# Based on https://lkml.org/lkml/2019/12/13/564.
+test1()
+{
+	local keyrings keycheck_line templates
+
+	tst_res TINFO "verifying key measurement for keyrings and \
+templates specified in ima policy file"
+
+	IMA_POLICY="$IMA_DIR/policy"
+	[ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY"
+
+	keycheck_line=$(grep "func=KEY_CHECK" $IMA_POLICY)
+	if [ -z "$keycheck_line" ]; then
+		tst_brk TCONF "ima policy does not specify \"func=KEY_CHECK\""
+	fi
+
+	if echo "$keycheck_line" | grep -q "*keyrings*"; then
+		tst_brk TCONF "ima policy does not specify a keyrings to check"
+	fi
+
+	keyrings=$(echo "$keycheck_line" | tr " " "\n" | grep "keyrings" | \
+		sed "s/\./\\\./g" | cut -d'=' -f2)
+	if [ -z "$keyrings" ]; then
+		tst_brk TCONF "ima policy has a keyring key-value specifier, \
+but no specified keyrings"
+	fi
+
+	templates=$(echo "$keycheck_line" | tr " " "\n" | grep "template" | \
+		cut -d'=' -f2)
+
+	grep -E "($templates)*($keyrings)" $ASCII_MEASUREMENTS | while read line
+	do
+		local digest expected_digest algorithm
+
+		digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2)
+		algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1)
+
+		echo "$line" | cut -d' ' -f6 | xxd -r -p > $TEST_FILE
+
+		expected_digest="$(compute_digest $algorithm $TEST_FILE)" || \
+			tst_brk TCONF "cannot compute digest for $algorithm"
+
+		if [ "$digest" != "$expected_digest" ]; then
+			tst_res TFAIL "incorrect digest was found for the \
+$(echo "$line" | cut -d' ' -f5) keyring"
+		fi
+	done
+
+	tst_res TPASS "specified keyrings were measured correctly"
+}
+
+tst_run
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
index 54237d688..0a58d021d 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh
@@ -12,6 +12,7 @@ TST_CNT=3
 TST_NEEDS_DEVICE=1
 
 . ima_setup.sh
+. compute_digest.sh
 
 setup()
 {
@@ -28,7 +29,7 @@ setup()
 	# parse digest index
 	# https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use
 	case "$template" in
-	ima|ima-ng|ima-sig) DIGEST_INDEX=4 ;;
+	ima|ima-ng|ima-sig|ima-buf) DIGEST_INDEX=4 ;;
 	*)
 		# using ima_template_fmt kernel parameter
 		local IFS="|"
@@ -46,40 +47,6 @@ setup()
 		"Cannot find digest index (template: '$template')"
 }
 
-# TODO: find support for rmd128 rmd256 rmd320 wp256 wp384 tgr128 tgr160
-compute_digest()
-{
-	local algorithm="$1"
-	local file="$2"
-	local digest
-
-	digest="$(${algorithm}sum $file 2>/dev/null | cut -f1 -d ' ')"
-	if [ -n "$digest" ]; then
-		echo "$digest"
-		return 0
-	fi
-
-	digest="$(openssl $algorithm $file 2>/dev/null | cut -f2 -d ' ')"
-	if [ -n "$digest" ]; then
-		echo "$digest"
-		return 0
-	fi
-
-	# uncommon ciphers
-	local arg="$algorithm"
-	case "$algorithm" in
-	tgr192) arg="tiger" ;;
-	wp512) arg="whirlpool" ;;
-	esac
-
-	digest="$(rdigest --$arg $file 2>/dev/null | cut -f1 -d ' ')"
-	if [ -n "$digest" ]; then
-		echo "$digest"
-		return 0
-	fi
-	return 1
-}
-
 ima_check()
 {
 	local delimiter=':'
-- 
2.25.1

[PATCH 2/2] IMA: Add a test to verify importing a certificate into keyring

[Lachlan Sneff]

Add an IMA measurement test that verifies that an x509 certificate
can be imported into the .ima keyring and measured correctly.

Signed-off-by: Lachlan Sneff <t-josne@linux.microsoft.com>
---
 .../security/integrity/ima/tests/ima_keys.sh  | 44 ++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
index 1b0dd0aed..6904fabfa 100644
--- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
@@ -7,7 +7,7 @@
 
 TST_NEEDS_CMDS="awk cut"
 TST_SETUP="setup"
-TST_CNT=1
+TST_CNT=2
 TST_NEEDS_DEVICE=1
 
 . ima_setup.sh
@@ -69,4 +69,46 @@ $(echo "$line" | cut -d' ' -f5) keyring"
 	tst_res TPASS "specified keyrings were measured correctly"
 }
 
+
+# Test that a cert can be imported into the ".ima" keyring correctly.
+test2() {
+	local keyring_id key_id
+	CERT_FILE="/etc/keys/x509_ima.der" # Default
+
+	[ -f $CERT_FILE ] || tst_brk TCONF "missing $CERT_FILE"
+
+	if ! openssl x509 -in $CERT_FILE -inform der > /dev/null; then
+		tst_brk TCONF "The suppled cert file ($CERT_FILE) is not \
+a valid x509 certificate"
+	fi
+
+	tst_res TINFO "adding a cert to the \".ima\" keyring ($CERT_FILE)"
+
+	keyring_id=$(sudo keyctl show %:.ima | sed -n 2p | \
+		sed 's/^[[:space:]]*//' | cut -d' ' -f1) || \
+		tst_btk TCONF "unable to retrieve .ima keyring id"
+
+	if ! tst_is_num	"$keyring_id"; then
+		tst_brk TCONF "unable to parse keyring id from keyring"
+	fi
+
+	sudo evmctl import $CERT_FILE "$keyring_id" > /dev/null || \
+		tst_brk TCONF "unable to import a cert into the .ima keyring"
+
+	grep -F ".ima" "$ASCII_MEASUREMENTS" | tail -n1 | cut -d' ' -f6 | \
+		xxd -r -p > $TEST_FILE || \
+		tst_brk TCONF "cert not found in ascii_runtime_measurements log"
+
+	if ! openssl x509 -in $TEST_FILE -inform der > /dev/null; then
+		tst_brk TCONF "The cert logged in ascii_runtime_measurements \
+($CERT_FILE) is not a valid x509 certificate"
+	fi
+
+	if cmp -s "$TEST_FILE" $CERT_FILE; then
+		tst_res TPASS "logged cert matches original cert"
+	else
+		tst_res TFAIL "logged cert does not match original cert"
+	fi
+}
+
 tst_run
-- 
2.25.1

Re: [PATCH 1/2] IMA: Add a test to verify measurment of keys

[Petr Vorel]

Hi Lachlan,

thank you for updating LTP!

I have few comments below.
Mostly just tiny details, which I could fix before merge.

> Add a testcase that verifies that the IMA subsystem has correctly
> measured keys added to keyrings specified in the IMA policy file.

> Additionally, add support for handling a new IMA template descriptor,
> namely ima-buf[1], in the IMA measurement tests.
Great, thanks!

> [1]: https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use

...
> +++ b/runtest/ima
> @@ -4,3 +4,4 @@ ima_policy ima_policy.sh
>  ima_tpm ima_tpm.sh
>  ima_violations ima_violations.sh
>  evm_overlay evm_overlay.sh
> +ima_keys ima_keys.sh
Please move evm_overlay after ima_keys.
EVM tests require specific configuration, that's why I put it on the end.
Or before ima_policy.sh (see comment below).

> diff --git a/testcases/kernel/security/integrity/ima/tests/compute_digest.sh b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
> new file mode 100644
Please use 755 (shell libraries which are sources can be 644, but this is
meant to be executed).

> index 000000000..85f6bf3da
> --- /dev/null
> +++ b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
Could you please move this to ima_setup.sh? That's the file we keep helper
functions. (BTW in LTP shell based tests it's usually foo_lib.sh, I named this
ima_setup.sh instead of ima_lib.sh, because it was first meant as tmpfs related
setup and still it does, although I later I added other helper functions there.

...
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
> @@ -0,0 +1,72 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2020 Microsoft Corporation
> +# Author: Lachlan Sneff <t-josne@linux.microsoft.com>
> +#
> +# Verify that keys are measured correctly based on policy.
> +
> +TST_NEEDS_CMDS="awk cut"
You missed xxd, which is IMHO less common than awk and cut, therefore it should
be specified.

> +TST_SETUP="setup"
> +TST_CNT=1
> +TST_NEEDS_DEVICE=1
> +
> +. ima_setup.sh
> +. compute_digest.sh
> +
> +setup()
> +{
> +    TEST_FILE="$PWD/test.txt"
This suggest that file tested in test2 (second commit) requires to be the
same. Is it true? If not, I'd set just local variable in each test function.
> +}
> +
> +# Based on https://lkml.org/lkml/2019/12/13/564.
Maybe also mention the commit?
450d0fd51564 ("IMA: Call workqueue functions to measure queued keys")

> +test1()
> +{
> +	local keyrings keycheck_line templates
> +
> +	tst_res TINFO "verifying key measurement for keyrings and \
> +templates specified in ima policy file"
Could you please keep string on single line? We prefer it over to 80 chars per
line rule (although I admit that super long string in check_policy_writable is awfull).
> +
> +	IMA_POLICY="$IMA_DIR/policy"
Could you please move IMA_POLICY to ima_setup.sh and remove the same definition
from ima_policy.sh?
> +	[ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY"
BTW to read IMA policy requires CONFIG_IMA_READ_POLICY=y. Maybe there could be
just some hint, because 

grep: /sys/kernel/security/ima/policy: Permission denied
ima_keys 1 TCONF: ima policy does not specify "func=KEY_CHECK"

Could we have here something like
cat $IMA_POLICY > /dev/null || tst_res TCONF "cannot read IMA policy (CONFIG_IMA_READ_POLICY=y required)"


> +
> +	keycheck_line=$(grep "func=KEY_CHECK" $IMA_POLICY)
> +	if [ -z "$keycheck_line" ]; then
> +		tst_brk TCONF "ima policy does not specify \"func=KEY_CHECK\""
> +	fi
Could we prepare policy example in
testcases/kernel/security/integrity/ima/datafiles/keycheck.policy?

I'm trying IMA tests to prepare themselves, otherwise most of the tests would be
TCONF. And the idea is that test has everything prepared when run as part of
runtest file or also on it's own. But here is getting complicated. Unless there is
CONFIG_IMA_WRITE_POLICY=y, we cannot load it in the test. And if we require
loading it before, then ima_policy.sh TCONF.

Not sure if we should have 2 versions of correct IMA policy.
in test2 in ima_policy.sh try to load first keycheck.policy and if it fails then
the old measure.policy. That way we would also avoid testing kernel version
(this functionality can be backported). We could also print support for new
feature. And try load keycheck.policy in ima_keys.sh, in case it's run on it's
own (not part of other tests in runtest file).

I understand if you don't want to play with this, I can add this after merging
your test. But providing keycheck.policy would help.

> +
> +	if echo "$keycheck_line" | grep -q "*keyrings*"; then
> +		tst_brk TCONF "ima policy does not specify a keyrings to check"
> +	fi
> +
> +	keyrings=$(echo "$keycheck_line" | tr " " "\n" | grep "keyrings" | \
> +		sed "s/\./\\\./g" | cut -d'=' -f2)
> +	if [ -z "$keyrings" ]; then
> +		tst_brk TCONF "ima policy has a keyring key-value specifier, \
> +but no specified keyrings"
+ also here put on single line.
> +	fi
> +
> +	templates=$(echo "$keycheck_line" | tr " " "\n" | grep "template" | \
> +		cut -d'=' -f2)
> +
> +	grep -E "($templates)*($keyrings)" $ASCII_MEASUREMENTS | while read line
> +	do
> +		local digest expected_digest algorithm
> +
> +		digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2)
> +		algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1)
> +
> +		echo "$line" | cut -d' ' -f6 | xxd -r -p > $TEST_FILE
> +
> +		expected_digest="$(compute_digest $algorithm $TEST_FILE)" || \
> +			tst_brk TCONF "cannot compute digest for $algorithm"
> +
> +		if [ "$digest" != "$expected_digest" ]; then
> +			tst_res TFAIL "incorrect digest was found for the \
> +$(echo "$line" | cut -d' ' -f5) keyring"
Here as well. Maybe add $keyring and use it as variable.
> +		fi
> +	done
> +
> +	tst_res TPASS "specified keyrings were measured correctly"
> +}
...

Kind regards,
Petr

Re: [PATCH 2/2] IMA: Add a test to verify importing a certificate into keyring

[Petr Vorel]

Hi Lachlan, Mimi,

@Mimi: I'd also appreciate you to review both commits.

> Add an IMA measurement test that verifies that an x509 certificate
> can be imported into the .ima keyring and measured correctly.

> Signed-off-by: Lachlan Sneff <t-josne@linux.microsoft.com>
> ---
>  .../security/integrity/ima/tests/ima_keys.sh  | 44 ++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)

> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
> index 1b0dd0aed..6904fabfa 100644
> --- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
> @@ -7,7 +7,7 @@

>  TST_NEEDS_CMDS="awk cut"
Not only xxd, but also keyctl needs to be here.
ALso it looks like you require openssl (unlike compute_digest where it's like
fallback and even here I was thinking about writing hint which packages are
required).
I'd also add cmp (probably less common than cut).
NOTE: Although not documented, we consider grep to be everywhere.

>  TST_SETUP="setup"
> -TST_CNT=1
> +TST_CNT=2
>  TST_NEEDS_DEVICE=1

Please put it here to allow overwrite it:
CERT_FILE="${CERT_FILE:-}/etc/keys/x509_ima.der"

>  . ima_setup.sh
> @@ -69,4 +69,46 @@ $(echo "$line" | cut -d' ' -f5) keyring"
>  	tst_res TPASS "specified keyrings were measured correctly"
>  }

> +
> +# Test that a cert can be imported into the ".ima" keyring correctly.
> +test2() {
> +	local keyring_id key_id
> +	CERT_FILE="/etc/keys/x509_ima.der" # Default
instead of here.
> +
> +	[ -f $CERT_FILE ] || tst_brk TCONF "missing $CERT_FILE"
> +
> +	if ! openssl x509 -in $CERT_FILE -inform der > /dev/null; then
> +		tst_brk TCONF "The suppled cert file ($CERT_FILE) is not \
> +a valid x509 certificate"
> +	fi
> +
> +	tst_res TINFO "adding a cert to the \".ima\" keyring ($CERT_FILE)"
nit: I personally would not quot .ima. I usually don't quot that much or use '
to help people grep, but that's not important.

> +
> +	keyring_id=$(sudo keyctl show %:.ima | sed -n 2p | \
> +		sed 's/^[[:space:]]*//' | cut -d' ' -f1) || \
> +		tst_btk TCONF "unable to retrieve .ima keyring id"
> +
> +	if ! tst_is_num	"$keyring_id"; then
> +		tst_brk TCONF "unable to parse keyring id from keyring"
> +	fi
> +
> +	sudo evmctl import $CERT_FILE "$keyring_id" > /dev/null || \
This test requires to be run with root (see TST_NEEDS_ROOT=1 in ima_setup.sh,
maybe I should have put the variables in each test to be this clear),
thus no need for sudo. Also you'd need to specify sudo in TST_NEEDS_CMDS
(precise check is needed as these tests can be run on some custom embedded
board, without any support.  Also some people test kernel with rapido.)

> +		tst_brk TCONF "unable to import a cert into the .ima keyring"
> +
> +	grep -F ".ima" "$ASCII_MEASUREMENTS" | tail -n1 | cut -d' ' -f6 | \
> +		xxd -r -p > $TEST_FILE || \
> +		tst_brk TCONF "cert not found in ascii_runtime_measurements log"
> +
> +	if ! openssl x509 -in $TEST_FILE -inform der > /dev/null; then
> +		tst_brk TCONF "The cert logged in ascii_runtime_measurements \
> +($CERT_FILE) is not a valid x509 certificate"
> +	fi
> +
> +	if cmp -s "$TEST_FILE" $CERT_FILE; then
> +		tst_res TPASS "logged cert matches original cert"
> +	else
> +		tst_res TFAIL "logged cert does not match original cert"
> +	fi
> +}
> +
>  tst_run

Again, thank for your patches!

Kind regards,
Petr

Re: [PATCH 1/2] IMA: Add a test to verify measurment of keys

[Lachlan Sneff]

Thank you for your review!

On 6/11/20 11:30 AM, Petr Vorel wrote:
> Hi Lachlan,
>
> thank you for updating LTP!
>
> I have few comments below.
> Mostly just tiny details, which I could fix before merge.
>
>> Add a testcase that verifies that the IMA subsystem has correctly
>> measured keys added to keyrings specified in the IMA policy file.
>> Additionally, add support for handling a new IMA template descriptor,
>> namely ima-buf[1], in the IMA measurement tests.
> Great, thanks!
:)
>> [1]: https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use
> ...
>> +++ b/runtest/ima
>> @@ -4,3 +4,4 @@ ima_policy ima_policy.sh
>>   ima_tpm ima_tpm.sh
>>   ima_violations ima_violations.sh
>>   evm_overlay evm_overlay.sh
>> +ima_keys ima_keys.sh
> Please move evm_overlay after ima_keys.
> EVM tests require specific configuration, that's why I put it on the end.
> Or before ima_policy.sh (see comment below).
Okay
>> diff --git a/testcases/kernel/security/integrity/ima/tests/compute_digest.sh b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
>> new file mode 100644
> Please use 755 (shell libraries which are sources can be 644, but this is
> meant to be executed).
This makes sense, sorry!
>> index 000000000..85f6bf3da
>> --- /dev/null
>> +++ b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh
> Could you please move this to ima_setup.sh? That's the file we keep helper
> functions. (BTW in LTP shell based tests it's usually foo_lib.sh, I named this
> ima_setup.sh instead of ima_lib.sh, because it was first meant as tmpfs related
> setup and still it does, although I later I added other helper functions there.

I wasn't sure what to do with compute_digest. I didn't want to duplicate 
it, so I added a

a new file for it. Adding it to ima_setup.sh is a solid idea.

> ...
>> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh
>> @@ -0,0 +1,72 @@
>> +#!/bin/sh
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +# Copyright (c) 2020 Microsoft Corporation
>> +# Author: Lachlan Sneff <t-josne@linux.microsoft.com>
>> +#
>> +# Verify that keys are measured correctly based on policy.
>> +
>> +TST_NEEDS_CMDS="awk cut"
> You missed xxd, which is IMHO less common than awk and cut, therefore it should
> be specified.
Good call.
>> +TST_SETUP="setup"
>> +TST_CNT=1
>> +TST_NEEDS_DEVICE=1
>> +
>> +. ima_setup.sh
>> +. compute_digest.sh
>> +
>> +setup()
>> +{
>> +    TEST_FILE="$PWD/test.txt"
> This suggest that file tested in test2 (second commit) requires to be the
> same. Is it true? If not, I'd set just local variable in each test function.
They don't need to be the same file. I'll just create a new one in each 
test.
>> +}
>> +
>> +# Based on https://lkml.org/lkml/2019/12/13/564.
> Maybe also mention the commit?
> 450d0fd51564 ("IMA: Call workqueue functions to measure queued keys")
Good call. I'll expand this comment.
>> +test1()
>> +{
>> +	local keyrings keycheck_line templates
>> +
>> +	tst_res TINFO "verifying key measurement for keyrings and \
>> +templates specified in ima policy file"
> Could you please keep string on single line? We prefer it over to 80 chars per
> line rule (although I admit that super long string in check_policy_writable is awfull).
Sounds good. I wasn't sure what to do with the long strings.
>> +
>> +	IMA_POLICY="$IMA_DIR/policy"
> Could you please move IMA_POLICY to ima_setup.sh and remove the same definition
> from ima_policy.sh?
Yep.
>> +	[ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY"
> BTW to read IMA policy requires CONFIG_IMA_READ_POLICY=y. Maybe there could be
> just some hint, because
>
> grep: /sys/kernel/security/ima/policy: Permission denied
> ima_keys 1 TCONF: ima policy does not specify "func=KEY_CHECK"
>
> Could we have here something like
> cat $IMA_POLICY > /dev/null || tst_res TCONF "cannot read IMA policy (CONFIG_IMA_READ_POLICY=y required)"
>
Didn't realize it wasn't necessarily readable. Will add that check.
>> +
>> +	keycheck_line=$(grep "func=KEY_CHECK" $IMA_POLICY)
>> +	if [ -z "$keycheck_line" ]; then
>> +		tst_brk TCONF "ima policy does not specify \"func=KEY_CHECK\""
>> +	fi
> Could we prepare policy example in
> testcases/kernel/security/integrity/ima/datafiles/keycheck.policy?
>
> I'm trying IMA tests to prepare themselves, otherwise most of the tests would be
> TCONF. And the idea is that test has everything prepared when run as part of
> runtest file or also on it's own. But here is getting complicated. Unless there is
> CONFIG_IMA_WRITE_POLICY=y, we cannot load it in the test. And if we require
> loading it before, then ima_policy.sh TCONF.
>
> Not sure if we should have 2 versions of correct IMA policy.
> in test2 in ima_policy.sh try to load first keycheck.policy and if it fails then
> the old measure.policy. That way we would also avoid testing kernel version
> (this functionality can be backported). We could also print support for new
> feature. And try load keycheck.policy in ima_keys.sh, in case it's run on it's
> own (not part of other tests in runtest file).
>
> I understand if you don't want to play with this, I can add this after merging
> your test. But providing keycheck.policy would help.

I'll supply a keycheck.policy in my updated patch, but I need to think 
more about what you've written here. Is the idea to have an example 
policy that the test can load if the kernel was built with 
CONFIG_IMA_WRITE_POLICY=y?

What should the test do in that case if the kernel wasn't build with a 
writable IMA policy?

>> +
>> +	if echo "$keycheck_line" | grep -q "*keyrings*"; then
>> +		tst_brk TCONF "ima policy does not specify a keyrings to check"
>> +	fi
>> +
>> +	keyrings=$(echo "$keycheck_line" | tr " " "\n" | grep "keyrings" | \
>> +		sed "s/\./\\\./g" | cut -d'=' -f2)
>> +	if [ -z "$keyrings" ]; then
>> +		tst_brk TCONF "ima policy has a keyring key-value specifier, \
>> +but no specified keyrings"
> + also here put on single line.
Yep
>> +	fi
>> +
>> +	templates=$(echo "$keycheck_line" | tr " " "\n" | grep "template" | \
>> +		cut -d'=' -f2)
>> +
>> +	grep -E "($templates)*($keyrings)" $ASCII_MEASUREMENTS | while read line
>> +	do
>> +		local digest expected_digest algorithm
>> +
>> +		digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2)
>> +		algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1)
>> +
>> +		echo "$line" | cut -d' ' -f6 | xxd -r -p > $TEST_FILE
>> +
>> +		expected_digest="$(compute_digest $algorithm $TEST_FILE)" || \
>> +			tst_brk TCONF "cannot compute digest for $algorithm"
>> +
>> +		if [ "$digest" != "$expected_digest" ]; then
>> +			tst_res TFAIL "incorrect digest was found for the \
>> +$(echo "$line" | cut -d' ' -f5) keyring"
> Here as well. Maybe add $keyring and use it as variable.
Okay
>> +		fi
>> +	done
>> +
>> +	tst_res TPASS "specified keyrings were measured correctly"
>> +}
> ...
>
> Kind regards,
> Petr

Thanks,

Lachlan