[LTP] [PATCH] IMA: Add test for dm-crypt measurement

[Petr Vorel]

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.

> Apologies for my ignorance.
> I will include it next time.
Thanks! No big deal :).

> > > 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.
> Sure. :)

...
> > > +### 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?

> Maybe I should create a directory ‘ima_device_mapper’ rather than
> ‘ima_dm_crypt’ - because there could be more DM targets like ‘crypt’.
> And I will place all the dm target policy files there.
> Does it sound good?
The directory must have the same name as the test filename without extension,
because that is $TST_ID, see tst_test.sh:

if [ -z "$TST_ID" ]; then
	_tst_filename=$(basename $0) || \
		tst_brk TCONF "Failed to set TST_ID from \$0 ('$0'), fix it with setting TST_ID before sourcing tst_test.sh"
	TST_ID=${_tst_filename%%.*}
fi
export TST_ID="$TST_ID"

...
	export TST_DATAROOT="$LTPROOT/testcases/data/$TST_ID"

Unfortunately that's the current limitation for LTP data files.

...
> > > +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

> I believe I do need to initialize res=0.
> Please see my comment below.
I'll reply there.

...
> > > +	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.
> Ok. I will see how I can generalize this. Thanks.
Thanks a lot!

...
> > BTW I plan to send v2 for ima_tpm.sh patch
> > https://patchwork.ozlabs.org/project/ltp/patch/20200527071434.28574-1-pvorel@suse.cz/
> > 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.

> Ok. I will take a look at your v2. But won’t take a dependency on it, for
> the time being.
+1 Thanks! (it's really less important than previous generalizations with
ima_keys.sh).

...
> > > +			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?
> Thanks for the feedback.
> The while loop invokes a sub-shell. And res=1 would mean the hash is
> found, and I fail if the hash is *not* found (res=0).
> With that, it’s little tricky to do what you are suggesting. But I will see
> what I can do to simplify the logic.
It's just a small detail, you can ignore it. But we started the discussion, thus
here is an explanation:
[ "$res" = 1 ] is not true for uninitialized $res. That wouldn't work for [ $res
-eq 1 ] and also didn't work [ $res = 1 ] ("bash: [: =: unary operator
expected"). The trick is that = operator compares stings but also does numeric
evaluation and also "$uninitialized_variable" evaluates as "" (unlike
$uninitialized_variable - without quotes).

Thus instead of 0 vs. 1 you can use uninitialized vs 1 (unlike C, where 0 vs 1
must be used e.g. variable must be initialized, unless it's static variable
which is automatically initialized to 0). We use it often shell (in LTP), have a
look at tst_net.sh and tst_test.sh

Kind regards,
Petr