RE: [PATCH v2 6/6] selftests/bpf: Add test for bpf_lsm_kernel_read_file()

[Roberto Sassu]

> From: Shuah Khan [mailto:skhan@xxxxxxxxxxxxxxxxxxx]
> Sent: Tuesday, February 15, 2022 5:11 PM
> On 2/15/22 5:40 AM, Roberto Sassu wrote:
> > Test the ability of bpf_lsm_kernel_read_file() to call the sleepable
> > functions bpf_ima_inode_hash() or bpf_ima_file_hash() to obtain a
> > measurement of a loaded IMA policy.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > ---
> >   tools/testing/selftests/bpf/ima_setup.sh      |  2 ++
> >   .../selftests/bpf/prog_tests/test_ima.c       |  3 +-
> >   tools/testing/selftests/bpf/progs/ima.c       | 28 ++++++++++++++++---
> >   3 files changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/ima_setup.sh
> b/tools/testing/selftests/bpf/ima_setup.sh
> > index 8e62581113a3..82530f19f85a 100755
> > --- a/tools/testing/selftests/bpf/ima_setup.sh
> > +++ b/tools/testing/selftests/bpf/ima_setup.sh
> > @@ -51,6 +51,7 @@ setup()
> >
> >   	ensure_mount_securityfs
> >   	echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" >
> ${IMA_POLICY_FILE}
> > +	echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" >
> ${mount_dir}/policy_test
> >   }
> >
> >   cleanup() {
> > @@ -74,6 +75,7 @@ run()
> >   	local mount_dir="${tmp_dir}/mnt"
> >   	local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> >
> > +	echo ${mount_dir}/policy_test > ${IMA_POLICY_FILE}
> >   	exec "${copied_bin_path}"
> >   }
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c
> b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> > index 62bf0e830453..c4a62d7b70df 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> > @@ -97,8 +97,9 @@ void test_test_ima(void)
> >   	/*
> >   	 * 1 sample with use_ima_file_hash = false
> >   	 * 2 samples with use_ima_file_hash = true (./ima_setup.sh, /bin/true)
> > +	 * 1 sample with use_ima_file_hash = true (IMA policy)
> >   	 */
> > -	ASSERT_EQ(err, 3, "num_samples_or_err");
> > +	ASSERT_EQ(err, 4, "num_samples_or_err");
> >   	ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash");
> >
> >   close_clean:
> > diff --git a/tools/testing/selftests/bpf/progs/ima.c
> b/tools/testing/selftests/bpf/progs/ima.c
> > index 9bb63f96cfc0..9b4c03f30a1c 100644
> > --- a/tools/testing/selftests/bpf/progs/ima.c
> > +++ b/tools/testing/selftests/bpf/progs/ima.c
> > @@ -20,8 +20,7 @@ char _license[] SEC("license") = "GPL";
> >
> >   bool use_ima_file_hash;
> >
> > -SEC("lsm.s/bprm_committed_creds")
> > -void BPF_PROG(ima, struct linux_binprm *bprm)
> > +static void ima_test_common(struct file *file)
> >   {
> >   	u64 ima_hash = 0;
> >   	u64 *sample;
> > @@ -31,10 +30,10 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
> >   	pid = bpf_get_current_pid_tgid() >> 32;
> >   	if (pid == monitored_pid) {
> >   		if (!use_ima_file_hash)
> > -			ret = bpf_ima_inode_hash(bprm->file->f_inode,
> &ima_hash,
> > +			ret = bpf_ima_inode_hash(file->f_inode, &ima_hash,
> >   						 sizeof(ima_hash));
> >   		else
> > -			ret = bpf_ima_file_hash(bprm->file, &ima_hash,
> > +			ret = bpf_ima_file_hash(file, &ima_hash,
> >   						sizeof(ima_hash));
> >   		if (ret < 0 || ima_hash == 0)
> 
> Is this considered an error? Does it make sense for this test to be
> void type and not return the error to its callers? One of the callers
> below seems to care for return values.

The user space side of the test (test_ima.c) seems to check the
number of samples obtained from the ring buffer. A failure here
would result in the sample not being sent to that component.

Another test, as you suggest, could be to ensure that the
kernel_read_file hook is able to deny operations. I would check
this in a separate test.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua 

> >   			return;
> > @@ -49,3 +48,24 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
> >
> >   	return;
> >   }
> > +
> > +SEC("lsm.s/bprm_committed_creds")
> > +void BPF_PROG(ima, struct linux_binprm *bprm)
> > +{
> > +	ima_test_common(bprm->file);
> > +}
> > +
> > +SEC("lsm.s/kernel_read_file")
> > +int BPF_PROG(kernel_read_file, struct file *file, enum kernel_read_file_id id,
> > +	     bool contents)
> > +{
> > +	if (!contents)
> > +		return 0;
> > +
> > +	if (id != READING_POLICY)
> > +		return 0;
> > +
> > +	ima_test_common(file);
> 
> This one here.
> 
> > +
> > +	return 0;
> > +}
> >
> 
> thanks,
> -- Shuah