Hi Tushar, > IMA subsystem supports measuring data from other kernel components > through func=CRITICAL_DATA policy 'critical_kernel_data_sources'. > This IMA policy can be set to measure the data coming from device-mapper > targets. This scenario needs test coverage. Thank you for your patch.
First, you haven't send this patch to LTP mailing list (https://lists.linux.it/listinfo/ltp). I Cc it, please do next time. > Add a testcase which verifies that the IMA subsystem correctly measures > the data provided by one such DM target - dm-crypt. > This series needs a kernel built on the following repo/branch/patches: > repo: > https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git > branch: next-integrity > commit d012a7190fc1 ("Linux 5.9-rc2") > And the following patch series should be applied in the order below: > 1. https://patchwork.kernel.org/patch/11709527/ > 2. https://patchwork.kernel.org/patch/11742047/ > 3. https://patchwork.kernel.org/patch/11743265/ > 4. https://patchwork.kernel.org/patch/11743715/ Thanks for a detailed info. > The kernel code required for this series (the patches above) > is still under review. This series will be ready to merge in LTP only > after the above patches are merged. > This series is based on the following commit in LTP branch: > commit a277498c08a7 ("IMA/ima_keys.sh: Enhance policy checks") > Signed-off-by: Tushar Sugandhi <[email protected]> > --- > runtest/ima | 1 + > .../kernel/security/integrity/ima/README.md | 20 +++ > .../integrity/ima/tests/ima_dm_crypt.sh | 118 ++++++++++++++++++ > 3 files changed, 139 insertions(+) > create mode 100755 > testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh > diff --git a/runtest/ima b/runtest/ima > index 5f4b4a7a1..123b6c8b0 100644 > --- a/runtest/ima > +++ b/runtest/ima > @@ -5,4 +5,5 @@ ima_tpm ima_tpm.sh > ima_violations ima_violations.sh > ima_keys ima_keys.sh > ima_kexec ima_kexec.sh > +ima_dm_crypt ima_dm_crypt.sh > evm_overlay evm_overlay.sh > diff --git a/testcases/kernel/security/integrity/ima/README.md > b/testcases/kernel/security/integrity/ima/README.md > index 68d046678..663c0b624 100644 > --- a/testcases/kernel/security/integrity/ima/README.md > +++ b/testcases/kernel/security/integrity/ima/README.md > @@ -37,6 +37,26 @@ see example in `kexec.policy`. > The test attempts to kexec the existing running kernel image. > To kexec a different kernel image export `IMA_KEXEC_IMAGE=<pathname>`. > +### IMA DM target (dm-crypt) measurement test > + > +To enable IMA to measure device-mapper target - dm-crypt, > +`ima_dm_crypt.sh` requires a readable IMA policy, as well as > +a loaded measure policy with > +`func=CRITICAL_DATA critical_kernel_data_sources=dm-crypt` Could you please also create ima_dm_crypt.policy file in testcases/kernel/security/integrity/ima/datafiles/ima_dm_crypt/ directory? > + > +As well as what's required for the IMA tests, dm-crypt measurement test > require > +reading the IMA policy allowed in the kernel configuration: > +``` > +CONFIG_IMA_READ_POLICY=y > +``` > + > +The following kernel configuration is also required. It enables compiling > +the device-mapper target module dm-crypt, which allows to create a device > +that transparently encrypts the data on it. > +``` > +CONFIG_DM_CRYPT > +``` > + > ## EVM tests > `evm_overlay.sh` requires a builtin IMA appraise tcb policy (e.g. > `ima_policy=appraise_tcb` > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh > b/testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh > new file mode 100755 > index 000000000..ee572ed01 > --- /dev/null > +++ b/testcases/kernel/security/integrity/ima/tests/ima_dm_crypt.sh > @@ -0,0 +1,118 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0-or-later > +# Copyright (c) 2020 Microsoft Corporation > +# Author: Tushar Sugandhi <[email protected]> > +# > +# Verify that DM target dm-crypt is measured correctly based on policy. > + > +TST_NEEDS_CMDS="grep cut sed tr dmsetup" You don't use tr, and omit xxd (copy paste error). + nit: this could be sorted. Thus: TST_NEEDS_CMDS="cut dmsetup grep sed xxd" > +TST_CNT=1 > +TST_NEEDS_DEVICE=1 > +TST_SETUP=setup > + > +. ima_setup.sh > + > +FUNC_CRIT_DATA='func=CRITICAL_DATA' > +TEMPLATE_BUF='template=ima-buf' > +REQUIRED_POLICY="^measure.*($FUNC_CRIT_DATA.*$TEMPLATE_BUF|$TEMPLATE_BUF.*$FUNC_CRIT_DATA)" > + > +setup() > +{ > + tst_res TINFO "inside setup" This is sort of debugging info. Please remove it. > + require_ima_policy_content "$REQUIRED_POLICY" '-E' > policy.txt > +} > + > +check_dm_crypt_policy() > +{ > + local pattern="$1" > + > + if ! grep -E "$pattern" policy.txt; then > + tst_res TCONF "IMA policy must specify $pattern, > $FUNC_CRIT_DATA, $TEMPLATE_BUF" > + return 1 > + fi > + return 0 > +} setup() and check_dm_crypt_policy() are the same as check_keys_policy() in ima_keys.sh. We could move $REQUIRED_POLICY handling into ima_setup() in ima_setup.sh. And check_{dm_crypt,keys}_policy() could be also moved to ima_setup.sh as check_test_policy(). NOTE, you need to use $TST_TMPDIR/policy.txt instead of policy.txt: see: https://patchwork.ozlabs.org/project/ltp/patch/[email protected]/ (I'm going to merge this one today) to fix problems on /tmp on tmpfs. (or simply just add temporary for testing TST_NEEDS_DEVICE=1 to the end of ima_setup.sh). > + > +test1() > +{ > + local dmcheck_lines i dm_targets templates > + local policy="critical_kernel_data_sources" > + local pattern='critical_kernel_data_sources=[^[:space:]]+' > + local tmp_file="tmp.txt" > + local tokens_file="tokens_file.txt" grep_file="grep_file.txt" > + local arg cmd key tgt_name > + local res=0 nit: local res Later you can check if [ "$res" = 1 ]; then > + > + tst_res TINFO "verifying dm target - dmcrypt gets measured correctly." > + > + check_dm_crypt_policy "$pattern" > $tmp_file || return > + > + dmcheck_lines=$(cat $tmp_file) > + dm_targets=$(for i in $dmcheck_lines; do echo "$i" | grep "$policy" | \ > + sed "s/\./\\\./g" | cut -d'=' -f2; done | sed > ':a;N;$!ba;s/\n/|/g') Again, copy paste from ima_keys.sh. Could this be generalized and moved to ima_setup.sh? See my hint below. > + if [ -z "$dm_targets" ]; then > + tst_res TCONF "IMA policy has a $policy key-value specifier, > but no specified sources." > + return > + fi > + > + templates=$(for i in $dmcheck_lines; do echo "$i" | grep "template" | \ > + cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g') > + > + tst_res TINFO "dm_targets: '$dm_targets'" > + tst_res TINFO "templates: '$templates'" > + > + tgt="crypt" > + key="faf453b4ee938cff2f0d2c869a0b743f59125c0a37f5bcd8f1dbbd911a78abaa" > + > + arg="'0 1953125 crypt aes-xts-plain64 " > + arg="$arg $key 0 " > + arg="$arg /dev/loop0 0 1 allow_discards'" > + tgt_name="test-crypt" > + cmd="dmsetup create $tgt_name --table $arg" > + > + tst_res TINFO "Executing: $cmd" > + eval $cmd Please no eval and TINFO. We have ROD() for this: ROD dmsetup create $tgt_name --table $arg" which also prints error if failed. > + > + grep -E "($templates)*($dm_targets)" $ASCII_MEASUREMENTS > $grep_file > + > + while read line > + do > + local act_digest exp_digest comp_digest algorithm > + > + act_digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2) > + algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1) > + dmtgt_evtname=$(echo "$line" | cut -d' ' -f5) Again, whole block is very similar to one from ima_keys.sh. Could this be generalized? Maybe this function would have callback for verification function? loop_measurements() { local pattern="$1" local target="$2" local ver_func="$3" shift 3 local lines line targets check_keys_policy "$pattern" > tmp || return lines=$(cat tmp) targets=$(for i in $lines; do echo "$i" | grep "$target" | \ sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g') if [ -z "$targets" ]; then tst_res TCONF "IMA policy has a $target key-value specifier, but no specified sources." return fi templates=$(for i in $lines; do echo "$i" | grep "template" | \ cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g') ... grep -E "($templates).*($targets)" $ASCII_MEASUREMENTS | while read line do ... $ver_func $@ done } BTW I plan to send v2 for ima_tpm.sh patch https://patchwork.ozlabs.org/project/ltp/patch/[email protected]/ Maybe this could use it somehow as well, but not required. Main reason was to use $DIGEST_INDEX (default 4), which would help to use tests also on ima_template_fmt. In the future, when reboot requirement is added into LTP API this could be used to tests more setup. But you can ignore it now. > + > + echo "$line" | cut -d' ' -f6 | xxd -r -p > $tokens_file > + plain_text=$(echo "$line" | cut -d' ' -f6 | xxd -r -p) > + > + #expected digest for $cmd This is obvious, please remove it. > + exp_digest="039d8ff71918608d585adca3e5aab2e3f41f84d6" > + comp_digest="$(compute_digest $algorithm $tokens_file)" || \ > + tst_brk TCONF "cannot compute digest for $algorithm" > + > + if [ "$act_digest" != "$comp_digest" ]; then > + tst_res TFAIL "Incorrect digest for ($dmtgt_evtname)." > + tst_res TFAIL "Expected digest:($comp_digest)." > + tst_res TFAIL "Actual digest:($act_digest)." This is wrong. There should be only single tst_res TFAIL message (errors are counted, thus there would be 3 failures instead of one. Use tst_res TFAIL "Incorrect digest for ($dmtgt_evtname): '$act_digest' (expected: '$comp_digest')" (keep it on single line to help grep the message.) nit: we usually don't put dot at the end of the sentence. And use lower case messages. And you're not consistent (you use both approaches). > + tst_res TINFO "Removing DM target $tgt_name." > + dmsetup remove $tgt_name > + return > + fi > + > + if [ "$act_digest" = "$exp_digest" ]; then > + res=1 Maybe also TFAIL and return here instead of using $res? > + fi > + > + done < $grep_file > + > + if [ $res -eq 1 ]; then if [ "$res" = 1 ]; then > + tst_res TPASS "dm-crypt target verification passed." > + else > + tst_res TFAIL "dm-crypt target verification failed." > + fi > + tst_res TINFO "Removing DM target $tgt_name." > + dmsetup remove $tgt_name Duplicity. Please, put it into cleanup function. > +} > + > +tst_run Kind regards, Petr -- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
