diff mbox series

[bpf-next,v3,3/3] bpf: Add a selftest for bpf_ima_inode_hash

Message ID 20201124151210.1081188-4-kpsingh@chromium.org (mailing list archive)
State New, archived
Headers show
Series Implement bpf_ima_inode_hash | expand

Commit Message

KP Singh Nov. 24, 2020, 3:12 p.m. UTC 
From: KP Singh <kpsingh@google.com>

The test does the following:

- Mounts a loopback filesystem and appends the IMA policy to measure
  executions only on this file-system. Restricting the IMA policy to a
  particular filesystem prevents a system-wide IMA policy change.
- Executes an executable copied to this loopback filesystem.
- Calls the bpf_ima_inode_hash in the bprm_committed_creds hook and
  checks if the call succeeded and checks if a hash was calculated.

The test shells out to the added ima_setup.sh script as the setup is
better handled in a shell script and is more complicated to do in the
test program or even shelling out individual commands from C.

The list of required configs (i.e. IMA, SECURITYFS,
IMA_{WRITE,READ}_POLICY) for running this test are also updated.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 tools/testing/selftests/bpf/config            |  4 +
 tools/testing/selftests/bpf/ima_setup.sh      | 80 +++++++++++++++++++
 .../selftests/bpf/prog_tests/test_ima.c       | 74 +++++++++++++++++
 tools/testing/selftests/bpf/progs/ima.c       | 28 +++++++
 4 files changed, 186 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/ima_setup.sh
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_ima.c
 create mode 100644 tools/testing/selftests/bpf/progs/ima.c

Comments

Yonghong Song Nov. 24, 2020, 6:07 p.m. UTC | #1 
On 11/24/20 7:12 AM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> The test does the following:
> 
> - Mounts a loopback filesystem and appends the IMA policy to measure
>    executions only on this file-system. Restricting the IMA policy to a
>    particular filesystem prevents a system-wide IMA policy change.
> - Executes an executable copied to this loopback filesystem.
> - Calls the bpf_ima_inode_hash in the bprm_committed_creds hook and
>    checks if the call succeeded and checks if a hash was calculated.
> 
> The test shells out to the added ima_setup.sh script as the setup is
> better handled in a shell script and is more complicated to do in the
> test program or even shelling out individual commands from C.
> 
> The list of required configs (i.e. IMA, SECURITYFS,
> IMA_{WRITE,READ}_POLICY) for running this test are also updated.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>

Ack from bpf perspective,

Acked-by: Yonghong Song <yhs@fb.com>

LSM/IMA experts may want to check ima_setup.sh as well.

> ---
>   tools/testing/selftests/bpf/config            |  4 +
>   tools/testing/selftests/bpf/ima_setup.sh      | 80 +++++++++++++++++++
>   .../selftests/bpf/prog_tests/test_ima.c       | 74 +++++++++++++++++
>   tools/testing/selftests/bpf/progs/ima.c       | 28 +++++++
>   4 files changed, 186 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/ima_setup.sh
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/test_ima.c
>   create mode 100644 tools/testing/selftests/bpf/progs/ima.c
> 
> diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> index 2118e23ac07a..365bf9771b07 100644
> --- a/tools/testing/selftests/bpf/config
> +++ b/tools/testing/selftests/bpf/config
> @@ -39,3 +39,7 @@ CONFIG_BPF_JIT=y
>   CONFIG_BPF_LSM=y
>   CONFIG_SECURITY=y
>   CONFIG_LIRC=y
> +CONFIG_IMA=y
> +CONFIG_SECURITYFS=y
> +CONFIG_IMA_WRITE_POLICY=y
> +CONFIG_IMA_READ_POLICY=y
> diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh
> new file mode 100644
> index 000000000000..15490ccc5e55
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/ima_setup.sh
> @@ -0,0 +1,80 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +set -u
> +
> +IMA_POLICY_FILE="/sys/kernel/security/ima/policy"
> +TEST_BINARY="/bin/true"
> +
> +usage()
> +{
> +        echo "Usage: $0 <setup|cleanup|run> <existing_tmp_dir>"
> +        exit 1
> +}
> +
> +setup()
> +{
> +        local tmp_dir="$1"
> +        local mount_img="${tmp_dir}/test.img"
> +        local mount_dir="${tmp_dir}/mnt"
> +        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> +        mkdir -p ${mount_dir}
> +
> +        dd if=/dev/zero of="${mount_img}" bs=1M count=10
> +
> +        local loop_device="$(losetup --find --show ${mount_img})"
> +
> +        mkfs.ext4 "${loop_device}"
> +        mount "${loop_device}" "${mount_dir}"
> +
> +        cp "${TEST_BINARY}" "${mount_dir}"
> +        local mount_uuid="$(blkid -s UUID -o value ${loop_device})"
> +        echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${IMA_POLICY_FILE}
> +}
> +
> +cleanup() {
> +        local tmp_dir="$1"
> +        local mount_img="${tmp_dir}/test.img"
> +        local mount_dir="${tmp_dir}/mnt"
> +
> +        local loop_devices=$(losetup -j ${mount_img} -O NAME --noheadings)
> +        for loop_dev in "${loop_devices}"; do
> +                losetup -d $loop_dev
> +        done
> +
> +        umount ${mount_dir}
> +        rm -rf ${tmp_dir}
> +}
> +
> +run()
> +{
> +        local tmp_dir="$1"
> +        local mount_dir="${tmp_dir}/mnt"
> +        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> +
> +        exec "${copied_bin_path}"
> +}
> +
> +main()
> +{
> +        [[ $# -ne 2 ]] && usage
> +
> +        local action="$1"
> +        local tmp_dir="$2"
> +
> +        [[ ! -d "${tmp_dir}" ]] && echo "Directory ${tmp_dir} doesn't exist" && exit 1
> +
> +        if [[ "${action}" == "setup" ]]; then
> +                setup "${tmp_dir}"
> +        elif [[ "${action}" == "cleanup" ]]; then
> +                cleanup "${tmp_dir}"
> +        elif [[ "${action}" == "run" ]]; then
> +                run "${tmp_dir}"
> +        else
> +                echo "Unknown action: ${action}"
> +                exit 1
> +        fi
> +}
> +
> +main "$@"
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> new file mode 100644
> index 000000000000..61fca681d524
[...]
Mimi Zohar Nov. 25, 2020, 2:20 a.m. UTC | #2 
On Tue, 2020-11-24 at 15:12 +0000, KP Singh wrote:
> diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh
> new file mode 100644
> index 000000000000..15490ccc5e55
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/ima_setup.sh
> @@ -0,0 +1,80 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +set -u
> +
> +IMA_POLICY_FILE="/sys/kernel/security/ima/policy"
> +TEST_BINARY="/bin/true"
> +
> +usage()
> +{
> +        echo "Usage: $0 <setup|cleanup|run> <existing_tmp_dir>"
> +        exit 1
> +}
> +
> +setup()
> +{
> +        local tmp_dir="$1"
> +        local mount_img="${tmp_dir}/test.img"
> +        local mount_dir="${tmp_dir}/mnt"
> +        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> +        mkdir -p ${mount_dir}
> +
> +        dd if=/dev/zero of="${mount_img}" bs=1M count=10
> +
> +        local loop_device="$(losetup --find --show ${mount_img})"
> +
> +        mkfs.ext4 "${loop_device}"
> +        mount "${loop_device}" "${mount_dir}"
> +
> +        cp "${TEST_BINARY}" "${mount_dir}"
> +        local mount_uuid="$(blkid -s UUID -o value ${loop_device})"
> +        echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${IMA_POLICY_FILE}

Anyone using IMA, normally define policy rules requiring the policy
itself to be signed.   Instead of writing the policy rules, write the
signed policy file pathname.  Refer to dracut commit 479b5cd9
("98integrity: support validating the IMA policy file signature").

Both enabling IMA_APPRAISE_REQUIRE_POLICY_SIGS and the builtin
"appraise_tcb" policy require loading a signed policy.

Mimi
KP Singh Nov. 25, 2020, 2:55 a.m. UTC | #3 
On Wed, Nov 25, 2020 at 3:20 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2020-11-24 at 15:12 +0000, KP Singh wrote:
> > diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh
> > new file mode 100644
> > index 000000000000..15490ccc5e55
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/ima_setup.sh
> > @@ -0,0 +1,80 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +set -e
> > +set -u
> > +
> > +IMA_POLICY_FILE="/sys/kernel/security/ima/policy"
> > +TEST_BINARY="/bin/true"
> > +
> > +usage()
> > +{
> > +        echo "Usage: $0 <setup|cleanup|run> <existing_tmp_dir>"
> > +        exit 1
> > +}
> > +
> > +setup()
> > +{
> > +        local tmp_dir="$1"
> > +        local mount_img="${tmp_dir}/test.img"
> > +        local mount_dir="${tmp_dir}/mnt"
> > +        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> > +        mkdir -p ${mount_dir}
> > +
> > +        dd if=/dev/zero of="${mount_img}" bs=1M count=10
> > +
> > +        local loop_device="$(losetup --find --show ${mount_img})"
> > +
> > +        mkfs.ext4 "${loop_device}"
> > +        mount "${loop_device}" "${mount_dir}"
> > +
> > +        cp "${TEST_BINARY}" "${mount_dir}"
> > +        local mount_uuid="$(blkid -s UUID -o value ${loop_device})"
> > +        echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${IMA_POLICY_FILE}
>
> Anyone using IMA, normally define policy rules requiring the policy
> itself to be signed.   Instead of writing the policy rules, write the

The goal of this self test is to not fully test the IMA functionality but check
if the BPF helper works and returns a hash with the minimal possible IMA
config dependencies. And it seems like we can accomplish this by simply
writing the policy to securityfs directly.

From what I noticed, IMA_APPRAISE_REQUIRE_POLICY_SIGS
requires configuring a lot of other kernel options
(IMA_APPRAISE, ASYMMETRIC_KEYS etc.) that seem
like too much for bpf self tests to depend on.

I guess we can independently add selftests for IMA  which represent
a more real IMA configuration.  Hope this sounds reasonable?

> signed policy file pathname.  Refer to dracut commit 479b5cd9
> ("98integrity: support validating the IMA policy file signature").
>
> Both enabling IMA_APPRAISE_REQUIRE_POLICY_SIGS and the builtin
> "appraise_tcb" policy require loading a signed policy.

Thanks for the pointers.

- KP

>



> Mimi
>
Mimi Zohar Nov. 25, 2020, 3:01 a.m. UTC | #4 
On Wed, 2020-11-25 at 03:55 +0100, KP Singh wrote:
> On Wed, Nov 25, 2020 at 3:20 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Tue, 2020-11-24 at 15:12 +0000, KP Singh wrote:
> > > diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh
> > > new file mode 100644
> > > index 000000000000..15490ccc5e55
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/ima_setup.sh
> > > @@ -0,0 +1,80 @@
> > > +#!/bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +
> > > +set -e
> > > +set -u
> > > +
> > > +IMA_POLICY_FILE="/sys/kernel/security/ima/policy"
> > > +TEST_BINARY="/bin/true"
> > > +
> > > +usage()
> > > +{
> > > +        echo "Usage: $0 <setup|cleanup|run> <existing_tmp_dir>"
> > > +        exit 1
> > > +}
> > > +
> > > +setup()
> > > +{
> > > +        local tmp_dir="$1"
> > > +        local mount_img="${tmp_dir}/test.img"
> > > +        local mount_dir="${tmp_dir}/mnt"
> > > +        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> > > +        mkdir -p ${mount_dir}
> > > +
> > > +        dd if=/dev/zero of="${mount_img}" bs=1M count=10
> > > +
> > > +        local loop_device="$(losetup --find --show ${mount_img})"
> > > +
> > > +        mkfs.ext4 "${loop_device}"
> > > +        mount "${loop_device}" "${mount_dir}"
> > > +
> > > +        cp "${TEST_BINARY}" "${mount_dir}"
> > > +        local mount_uuid="$(blkid -s UUID -o value ${loop_device})"
> > > +        echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${IMA_POLICY_FILE}
> >
> > Anyone using IMA, normally define policy rules requiring the policy
> > itself to be signed.   Instead of writing the policy rules, write the
> 
> The goal of this self test is to not fully test the IMA functionality but check
> if the BPF helper works and returns a hash with the minimal possible IMA
> config dependencies. And it seems like we can accomplish this by simply
> writing the policy to securityfs directly.
> 
> From what I noticed, IMA_APPRAISE_REQUIRE_POLICY_SIGS
> requires configuring a lot of other kernel options
> (IMA_APPRAISE, ASYMMETRIC_KEYS etc.) that seem
> like too much for bpf self tests to depend on.
> 
> I guess we can independently add selftests for IMA  which represent
> a more real IMA configuration.  Hope this sounds reasonable?

Sure.  My point was that writing the policy rule might fail.

Mimi
> 
> > signed policy file pathname.  Refer to dracut commit 479b5cd9
> > ("98integrity: support validating the IMA policy file signature").
> >
> > Both enabling IMA_APPRAISE_REQUIRE_POLICY_SIGS and the builtin
> > "appraise_tcb" policy require loading a signed policy.
> 
> Thanks for the pointers.
Mimi Zohar Nov. 25, 2020, 12:27 p.m. UTC | #5 
On Tue, 2020-11-24 at 15:12 +0000, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> The test does the following:
> 
> - Mounts a loopback filesystem and appends the IMA policy to measure
>   executions only on this file-system. Restricting the IMA policy to a
>   particular filesystem prevents a system-wide IMA policy change.
> - Executes an executable copied to this loopback filesystem.
> - Calls the bpf_ima_inode_hash in the bprm_committed_creds hook and
>   checks if the call succeeded and checks if a hash was calculated.
> 
> The test shells out to the added ima_setup.sh script as the setup is
> better handled in a shell script and is more complicated to do in the
> test program or even shelling out individual commands from C.
> 
> The list of required configs (i.e. IMA, SECURITYFS,
> IMA_{WRITE,READ}_POLICY) for running this test are also updated.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>

Suggested-by: Mimi Zohar <zohar@linux.ibm.com> (limit policy rule to
loopback mount)

> ---
>  tools/testing/selftests/bpf/config            |  4 +
>  tools/testing/selftests/bpf/ima_setup.sh      | 80 +++++++++++++++++++
>  .../selftests/bpf/prog_tests/test_ima.c       | 74 +++++++++++++++++
>  tools/testing/selftests/bpf/progs/ima.c       | 28 +++++++
>  4 files changed, 186 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/ima_setup.sh
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_ima.c
>  create mode 100644 tools/testing/selftests/bpf/progs/ima.c
> 
> diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> index 2118e23ac07a..365bf9771b07 100644
> --- a/tools/testing/selftests/bpf/config
> +++ b/tools/testing/selftests/bpf/config
> @@ -39,3 +39,7 @@ CONFIG_BPF_JIT=y
>  CONFIG_BPF_LSM=y
>  CONFIG_SECURITY=y
>  CONFIG_LIRC=y
> +CONFIG_IMA=y
> +CONFIG_SECURITYFS=y
> +CONFIG_IMA_WRITE_POLICY=y
> +CONFIG_IMA_READ_POLICY=y
> diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh
> new file mode 100644
> index 000000000000..15490ccc5e55
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/ima_setup.sh
> @@ -0,0 +1,80 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +set -u
> +
> +IMA_POLICY_FILE="/sys/kernel/security/ima/policy"
> +TEST_BINARY="/bin/true"
> +
> +usage()
> +{
> +        echo "Usage: $0 <setup|cleanup|run> <existing_tmp_dir>"
> +        exit 1
> +}
> +
> +setup()
> +{
> +        local tmp_dir="$1"
> +        local mount_img="${tmp_dir}/test.img"
> +        local mount_dir="${tmp_dir}/mnt"
> +        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> +        mkdir -p ${mount_dir}
> +
> +        dd if=/dev/zero of="${mount_img}" bs=1M count=10
> +
> +        local loop_device="$(losetup --find --show ${mount_img})"
> +
> +        mkfs.ext4 "${loop_device}"
> +        mount "${loop_device}" "${mount_dir}"
> +
> +        cp "${TEST_BINARY}" "${mount_dir}"
> +        local mount_uuid="$(blkid -s UUID -o value ${loop_device})"
> +        echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${IMA_POLICY_FILE}
> +}
> +
> +cleanup() {
> +        local tmp_dir="$1"
> +        local mount_img="${tmp_dir}/test.img"
> +        local mount_dir="${tmp_dir}/mnt"
> +
> +        local loop_devices=$(losetup -j ${mount_img} -O NAME --noheadings)
> +        for loop_dev in "${loop_devices}"; do
> +                losetup -d $loop_dev
> +        done
> +
> +        umount ${mount_dir}
> +        rm -rf ${tmp_dir}
> +}
> +
> +run()
> +{
> +        local tmp_dir="$1"
> +        local mount_dir="${tmp_dir}/mnt"
> +        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> +
> +        exec "${copied_bin_path}"
> +}
> +
> +main()
> +{
> +        [[ $# -ne 2 ]] && usage
> +
> +        local action="$1"
> +        local tmp_dir="$2"
> +
> +        [[ ! -d "${tmp_dir}" ]] && echo "Directory ${tmp_dir} doesn't exist" && exit 1
> +
> +        if [[ "${action}" == "setup" ]]; then
> +                setup "${tmp_dir}"
> +        elif [[ "${action}" == "cleanup" ]]; then
> +                cleanup "${tmp_dir}"
> +        elif [[ "${action}" == "run" ]]; then
> +                run "${tmp_dir}"
> +        else
> +                echo "Unknown action: ${action}"
> +                exit 1
> +        fi
> +}
> +
> +main "$@"
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> new file mode 100644
> index 000000000000..61fca681d524
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright (C) 2020 Google LLC.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <sys/wait.h>
> +#include <test_progs.h>
> +
> +#include "ima.skel.h"
> +
> +static int run_measured_process(const char *measured_dir, u32 *monitored_pid)
> +{
> +	int child_pid, child_status;
> +
> +	child_pid = fork();
> +	if (child_pid == 0) {
> +		*monitored_pid = getpid();
> +		execlp("./ima_setup.sh", "./ima_setup.sh", "run", measured_dir,
> +		       NULL);
> +		exit(errno);
> +
> +	} else if (child_pid > 0) {
> +		waitpid(child_pid, &child_status, 0);
> +		return WEXITSTATUS(child_status);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +void test_test_ima(void)
> +{
> +	char measured_dir_template[] = "/tmp/ima_measuredXXXXXX";
> +	const char *measured_dir;
> +	char cmd[256];
> +
> +	int err, duration = 0;
> +	struct ima *skel = NULL;
> +
> +	skel = ima__open_and_load();
> +	if (CHECK(!skel, "skel_load", "skeleton failed\n"))
> +		goto close_prog;
> +
> +	err = ima__attach(skel);
> +	if (CHECK(err, "attach", "attach failed: %d\n", err))
> +		goto close_prog;
> +
> +	measured_dir = mkdtemp(measured_dir_template);
> +	if (CHECK(measured_dir == NULL, "mkdtemp", "err %d\n", errno))
> +		goto close_prog;
> +
> +	snprintf(cmd, sizeof(cmd), "./ima_setup.sh setup %s", measured_dir);
> +	if (CHECK_FAIL(system(cmd)))
> +		goto close_clean;
> +
> +	err = run_measured_process(measured_dir, &skel->bss->monitored_pid);
> +	if (CHECK(err, "run_measured_process", "err = %d\n", err))
> +		goto close_clean;
> +
> +	CHECK(skel->data->ima_hash_ret < 0, "ima_hash_ret",
> +	      "ima_hash_ret = %ld\n", skel->data->ima_hash_ret);
> +
> +	CHECK(skel->bss->ima_hash == 0, "ima_hash",
> +	      "ima_hash = %lu\n", skel->bss->ima_hash);
> +
> +close_clean:
> +	snprintf(cmd, sizeof(cmd), "./ima_setup.sh cleanup %s", measured_dir);
> +	CHECK_FAIL(system(cmd));
> +close_prog:
> +	ima__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c
> new file mode 100644
> index 000000000000..86b21aff4bc5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/ima.c
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright 2020 Google LLC.
> + */
> +
> +#include "vmlinux.h"
> +#include <errno.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +long ima_hash_ret = -1;
> +u64 ima_hash = 0;
> +u32 monitored_pid = 0;
> +
> +char _license[] SEC("license") = "GPL";
> +
> +SEC("lsm.s/bprm_committed_creds")
> +int BPF_PROG(ima, struct linux_binprm *bprm)
> +{
> +	u32 pid = bpf_get_current_pid_tgid() >> 32;
> +
> +	if (pid == monitored_pid)
> +		ima_hash_ret = bpf_ima_inode_hash(bprm->file->f_inode,
> +						  &ima_hash, sizeof(ima_hash));
> +
> +	return 0;
> +}
Yonghong Song Nov. 26, 2020, 6:27 a.m. UTC | #6 
On 11/24/20 7:12 AM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> The test does the following:
> 
> - Mounts a loopback filesystem and appends the IMA policy to measure
>    executions only on this file-system. Restricting the IMA policy to a
>    particular filesystem prevents a system-wide IMA policy change.
> - Executes an executable copied to this loopback filesystem.
> - Calls the bpf_ima_inode_hash in the bprm_committed_creds hook and
>    checks if the call succeeded and checks if a hash was calculated.
> 
> The test shells out to the added ima_setup.sh script as the setup is
> better handled in a shell script and is more complicated to do in the
> test program or even shelling out individual commands from C.
> 
> The list of required configs (i.e. IMA, SECURITYFS,
> IMA_{WRITE,READ}_POLICY) for running this test are also updated.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>   tools/testing/selftests/bpf/config            |  4 +
>   tools/testing/selftests/bpf/ima_setup.sh      | 80 +++++++++++++++++++
>   .../selftests/bpf/prog_tests/test_ima.c       | 74 +++++++++++++++++
>   tools/testing/selftests/bpf/progs/ima.c       | 28 +++++++
>   4 files changed, 186 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/ima_setup.sh
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/test_ima.c
>   create mode 100644 tools/testing/selftests/bpf/progs/ima.c
> 
> diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> index 2118e23ac07a..365bf9771b07 100644
> --- a/tools/testing/selftests/bpf/config
> +++ b/tools/testing/selftests/bpf/config
> @@ -39,3 +39,7 @@ CONFIG_BPF_JIT=y
>   CONFIG_BPF_LSM=y
>   CONFIG_SECURITY=y
>   CONFIG_LIRC=y
> +CONFIG_IMA=y
> +CONFIG_SECURITYFS=y
> +CONFIG_IMA_WRITE_POLICY=y
> +CONFIG_IMA_READ_POLICY=y
> diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh
> new file mode 100644
> index 000000000000..15490ccc5e55
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/ima_setup.sh
> @@ -0,0 +1,80 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +set -u
> +
> +IMA_POLICY_FILE="/sys/kernel/security/ima/policy"
> +TEST_BINARY="/bin/true"
> +
> +usage()
> +{
> +        echo "Usage: $0 <setup|cleanup|run> <existing_tmp_dir>"
> +        exit 1
> +}
> +
> +setup()
> +{
> +        local tmp_dir="$1"
> +        local mount_img="${tmp_dir}/test.img"
> +        local mount_dir="${tmp_dir}/mnt"
> +        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> +        mkdir -p ${mount_dir}
> +
> +        dd if=/dev/zero of="${mount_img}" bs=1M count=10
> +
> +        local loop_device="$(losetup --find --show ${mount_img})"
> +
> +        mkfs.ext4 "${loop_device}"
> +        mount "${loop_device}" "${mount_dir}"
> +
> +        cp "${TEST_BINARY}" "${mount_dir}"
> +        local mount_uuid="$(blkid -s UUID -o value ${loop_device})"
> +        echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${IMA_POLICY_FILE}
> +}
> +
> +cleanup() {
> +        local tmp_dir="$1"
> +        local mount_img="${tmp_dir}/test.img"
> +        local mount_dir="${tmp_dir}/mnt"
> +
> +        local loop_devices=$(losetup -j ${mount_img} -O NAME --noheadings)
> +        for loop_dev in "${loop_devices}"; do
> +                losetup -d $loop_dev
> +        done
> +
> +        umount ${mount_dir}
> +        rm -rf ${tmp_dir}
> +}
> +
> +run()
> +{
> +        local tmp_dir="$1"
> +        local mount_dir="${tmp_dir}/mnt"
> +        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> +
> +        exec "${copied_bin_path}"
> +}
> +
> +main()
> +{
> +        [[ $# -ne 2 ]] && usage
> +
> +        local action="$1"
> +        local tmp_dir="$2"
> +
> +        [[ ! -d "${tmp_dir}" ]] && echo "Directory ${tmp_dir} doesn't exist" && exit 1
> +
> +        if [[ "${action}" == "setup" ]]; then
> +                setup "${tmp_dir}"
> +        elif [[ "${action}" == "cleanup" ]]; then
> +                cleanup "${tmp_dir}"
> +        elif [[ "${action}" == "run" ]]; then
> +                run "${tmp_dir}"
> +        else
> +                echo "Unknown action: ${action}"
> +                exit 1
> +        fi
> +}
> +
> +main "$@"


> diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> new file mode 100644
> index 000000000000..61fca681d524
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright (C) 2020 Google LLC.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <sys/wait.h>
> +#include <test_progs.h>
> +
> +#include "ima.skel.h"
> +
> +static int run_measured_process(const char *measured_dir, u32 *monitored_pid)
> +{
> +	int child_pid, child_status;
> +
> +	child_pid = fork();
> +	if (child_pid == 0) {
> +		*monitored_pid = getpid();
> +		execlp("./ima_setup.sh", "./ima_setup.sh", "run", measured_dir,
> +		       NULL);
> +		exit(errno);

Running test_progs-no-alu32, the test failed as:

root@arch-fb-vm1:~/net-next/net-next/tools/testing/selftests/bpf 
./test_progs-no_alu32 -t test_ima 

sh: ./ima_setup.sh: No such file or directory 

sh: ./ima_setup.sh: No such file or directory 

test_test_ima:PASS:skel_load 0 nsec 

test_test_ima:PASS:attach 0 nsec 

test_test_ima:PASS:mkdtemp 0 nsec 

test_test_ima:FAIL:56 

test_test_ima:FAIL:71 

#114 test_ima:FAIL 

Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED

Although the file is indeed in this directory:
root@arch-fb-vm1:~/net-next/net-next/tools/testing/selftests/bpf ls 
ima_setup.sh
ima_setup.sh

I think the execution actually tries to get file from
no_alu32 directory to avoid reusing the same files in
.../testing/selftests/bpf for -mcpu=v3 purpose.

The following change, which copies ima_setup.sh to
no_alu32 directory, seems fixing the issue:

TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c 
     \
                          network_helpers.c testing_helpers.c            \
                          btf_helpers.c  flow_dissector_load.h
  TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read                          \
+                      ima_setup.sh                                     \
                        $(wildcard progs/btf_dump_test_case_*.c)
  TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
  TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)

Could you do a followup on this?

> +
> +	} else if (child_pid > 0) {
> +		waitpid(child_pid, &child_status, 0);
> +		return WEXITSTATUS(child_status);
> +	}
> +
> +	return -EINVAL;
> +}
> +
[...]
KP Singh Nov. 26, 2020, 3:18 p.m. UTC | #7 
[...]

> > +             exit(errno);
>
> Running test_progs-no-alu32, the test failed as:
>
> root@arch-fb-vm1:~/net-next/net-next/tools/testing/selftests/bpf
> ./test_progs-no_alu32 -t test_ima

Note to self: Also start testing test_progs-no_alu32

>
> sh: ./ima_setup.sh: No such file or directory
>
> sh: ./ima_setup.sh: No such file or directory
>
> test_test_ima:PASS:skel_load 0 nsec
>
> test_test_ima:PASS:attach 0 nsec
>
> test_test_ima:PASS:mkdtemp 0 nsec
>
> test_test_ima:FAIL:56
>
> test_test_ima:FAIL:71
>
> #114 test_ima:FAIL
>
> Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>
> Although the file is indeed in this directory:
> root@arch-fb-vm1:~/net-next/net-next/tools/testing/selftests/bpf ls
> ima_setup.sh
> ima_setup.sh
>
> I think the execution actually tries to get file from
> no_alu32 directory to avoid reusing the same files in
> .../testing/selftests/bpf for -mcpu=v3 purpose.
>
> The following change, which copies ima_setup.sh to
> no_alu32 directory, seems fixing the issue:

Thanks!

>
> TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c
>      \
>                           network_helpers.c testing_helpers.c            \
>                           btf_helpers.c  flow_dissector_load.h
>   TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read                          \
> +                      ima_setup.sh                                     \
>                         $(wildcard progs/btf_dump_test_case_*.c)
>   TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
>   TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
>
> Could you do a followup on this?

Yes, I will send out a fix today.

- KP
Andrii Nakryiko Nov. 27, 2020, 4:29 a.m. UTC | #8 
On Tue, Nov 24, 2020 at 7:16 AM KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> The test does the following:
>
> - Mounts a loopback filesystem and appends the IMA policy to measure
>   executions only on this file-system. Restricting the IMA policy to a
>   particular filesystem prevents a system-wide IMA policy change.
> - Executes an executable copied to this loopback filesystem.
> - Calls the bpf_ima_inode_hash in the bprm_committed_creds hook and
>   checks if the call succeeded and checks if a hash was calculated.
>
> The test shells out to the added ima_setup.sh script as the setup is
> better handled in a shell script and is more complicated to do in the
> test program or even shelling out individual commands from C.
>
> The list of required configs (i.e. IMA, SECURITYFS,
> IMA_{WRITE,READ}_POLICY) for running this test are also updated.
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>  tools/testing/selftests/bpf/config            |  4 +
>  tools/testing/selftests/bpf/ima_setup.sh      | 80 +++++++++++++++++++
>  .../selftests/bpf/prog_tests/test_ima.c       | 74 +++++++++++++++++
>  tools/testing/selftests/bpf/progs/ima.c       | 28 +++++++
>  4 files changed, 186 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/ima_setup.sh
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_ima.c
>  create mode 100644 tools/testing/selftests/bpf/progs/ima.c
>

[...]

> +cleanup() {
> +        local tmp_dir="$1"
> +        local mount_img="${tmp_dir}/test.img"
> +        local mount_dir="${tmp_dir}/mnt"
> +
> +        local loop_devices=$(losetup -j ${mount_img} -O NAME --noheadings)

libbpf and kernel-patches CIs are using BusyBox environment which has
losetup that doesn't support -j option. Is there some way to work
around that? What we have is this:

BusyBox v1.31.1 () multi-call binary.

Usage: losetup [-rP] [-o OFS] {-f|LOOPDEV} FILE: associate loop devices

    losetup -c LOOPDEV: reread file size

    losetup -d LOOPDEV: disassociate

    losetup -a: show status

    losetup -f: show next free loop device

    -o OFS    Start OFS bytes into FILE

    -P    Scan for partitions

    -r    Read-only

    -f    Show/use next free loop device


> +        for loop_dev in "${loop_devices}"; do
> +                losetup -d $loop_dev
> +        done
> +
> +        umount ${mount_dir}
> +        rm -rf ${tmp_dir}
> +}
> +

[...]
KP Singh Nov. 27, 2020, 1:09 p.m. UTC | #9 
On Fri, Nov 27, 2020 at 5:29 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Nov 24, 2020 at 7:16 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >

[...]

>
> > +cleanup() {
> > +        local tmp_dir="$1"
> > +        local mount_img="${tmp_dir}/test.img"
> > +        local mount_dir="${tmp_dir}/mnt"
> > +
> > +        local loop_devices=$(losetup -j ${mount_img} -O NAME --noheadings)
>
> libbpf and kernel-patches CIs are using BusyBox environment which has
> losetup that doesn't support -j option. Is there some way to work
> around that? What we have is this:
>
> BusyBox v1.31.1 () multi-call binary.
>
> Usage: losetup [-rP] [-o OFS] {-f|LOOPDEV} FILE: associate loop devices
>
>     losetup -c LOOPDEV: reread file size
>
>     losetup -d LOOPDEV: disassociate
>
>     losetup -a: show status

I can try to grep and parse the status output as a fallback. Will send another
fix.

- KP

>
>     losetup -f: show next free loop device
>
>     -o OFS    Start OFS bytes into FILE
>
>     -P    Scan for partitions
>
>     -r    Read-only
>
>     -f    Show/use next free loop device
>
>
> > +        for loop_dev in "${loop_devices}"; do

[...]
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index 2118e23ac07a..365bf9771b07 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -39,3 +39,7 @@  CONFIG_BPF_JIT=y
 CONFIG_BPF_LSM=y
 CONFIG_SECURITY=y
 CONFIG_LIRC=y
+CONFIG_IMA=y
+CONFIG_SECURITYFS=y
+CONFIG_IMA_WRITE_POLICY=y
+CONFIG_IMA_READ_POLICY=y
diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh
new file mode 100644
index 000000000000..15490ccc5e55
--- /dev/null
+++ b/tools/testing/selftests/bpf/ima_setup.sh
@@ -0,0 +1,80 @@ 
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+set -u
+
+IMA_POLICY_FILE="/sys/kernel/security/ima/policy"
+TEST_BINARY="/bin/true"
+
+usage()
+{
+        echo "Usage: $0 <setup|cleanup|run> <existing_tmp_dir>"
+        exit 1
+}
+
+setup()
+{
+        local tmp_dir="$1"
+        local mount_img="${tmp_dir}/test.img"
+        local mount_dir="${tmp_dir}/mnt"
+        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
+        mkdir -p ${mount_dir}
+
+        dd if=/dev/zero of="${mount_img}" bs=1M count=10
+
+        local loop_device="$(losetup --find --show ${mount_img})"
+
+        mkfs.ext4 "${loop_device}"
+        mount "${loop_device}" "${mount_dir}"
+
+        cp "${TEST_BINARY}" "${mount_dir}"
+        local mount_uuid="$(blkid -s UUID -o value ${loop_device})"
+        echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${IMA_POLICY_FILE}
+}
+
+cleanup() {
+        local tmp_dir="$1"
+        local mount_img="${tmp_dir}/test.img"
+        local mount_dir="${tmp_dir}/mnt"
+
+        local loop_devices=$(losetup -j ${mount_img} -O NAME --noheadings)
+        for loop_dev in "${loop_devices}"; do
+                losetup -d $loop_dev
+        done
+
+        umount ${mount_dir}
+        rm -rf ${tmp_dir}
+}
+
+run()
+{
+        local tmp_dir="$1"
+        local mount_dir="${tmp_dir}/mnt"
+        local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
+
+        exec "${copied_bin_path}"
+}
+
+main()
+{
+        [[ $# -ne 2 ]] && usage
+
+        local action="$1"
+        local tmp_dir="$2"
+
+        [[ ! -d "${tmp_dir}" ]] && echo "Directory ${tmp_dir} doesn't exist" && exit 1
+
+        if [[ "${action}" == "setup" ]]; then
+                setup "${tmp_dir}"
+        elif [[ "${action}" == "cleanup" ]]; then
+                cleanup "${tmp_dir}"
+        elif [[ "${action}" == "run" ]]; then
+                run "${tmp_dir}"
+        else
+                echo "Unknown action: ${action}"
+                exit 1
+        fi
+}
+
+main "$@"
diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c
new file mode 100644
index 000000000000..61fca681d524
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c
@@ -0,0 +1,74 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/wait.h>
+#include <test_progs.h>
+
+#include "ima.skel.h"
+
+static int run_measured_process(const char *measured_dir, u32 *monitored_pid)
+{
+	int child_pid, child_status;
+
+	child_pid = fork();
+	if (child_pid == 0) {
+		*monitored_pid = getpid();
+		execlp("./ima_setup.sh", "./ima_setup.sh", "run", measured_dir,
+		       NULL);
+		exit(errno);
+
+	} else if (child_pid > 0) {
+		waitpid(child_pid, &child_status, 0);
+		return WEXITSTATUS(child_status);
+	}
+
+	return -EINVAL;
+}
+
+void test_test_ima(void)
+{
+	char measured_dir_template[] = "/tmp/ima_measuredXXXXXX";
+	const char *measured_dir;
+	char cmd[256];
+
+	int err, duration = 0;
+	struct ima *skel = NULL;
+
+	skel = ima__open_and_load();
+	if (CHECK(!skel, "skel_load", "skeleton failed\n"))
+		goto close_prog;
+
+	err = ima__attach(skel);
+	if (CHECK(err, "attach", "attach failed: %d\n", err))
+		goto close_prog;
+
+	measured_dir = mkdtemp(measured_dir_template);
+	if (CHECK(measured_dir == NULL, "mkdtemp", "err %d\n", errno))
+		goto close_prog;
+
+	snprintf(cmd, sizeof(cmd), "./ima_setup.sh setup %s", measured_dir);
+	if (CHECK_FAIL(system(cmd)))
+		goto close_clean;
+
+	err = run_measured_process(measured_dir, &skel->bss->monitored_pid);
+	if (CHECK(err, "run_measured_process", "err = %d\n", err))
+		goto close_clean;
+
+	CHECK(skel->data->ima_hash_ret < 0, "ima_hash_ret",
+	      "ima_hash_ret = %ld\n", skel->data->ima_hash_ret);
+
+	CHECK(skel->bss->ima_hash == 0, "ima_hash",
+	      "ima_hash = %lu\n", skel->bss->ima_hash);
+
+close_clean:
+	snprintf(cmd, sizeof(cmd), "./ima_setup.sh cleanup %s", measured_dir);
+	CHECK_FAIL(system(cmd));
+close_prog:
+	ima__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c
new file mode 100644
index 000000000000..86b21aff4bc5
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/ima.c
@@ -0,0 +1,28 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2020 Google LLC.
+ */
+
+#include "vmlinux.h"
+#include <errno.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+long ima_hash_ret = -1;
+u64 ima_hash = 0;
+u32 monitored_pid = 0;
+
+char _license[] SEC("license") = "GPL";
+
+SEC("lsm.s/bprm_committed_creds")
+int BPF_PROG(ima, struct linux_binprm *bprm)
+{
+	u32 pid = bpf_get_current_pid_tgid() >> 32;
+
+	if (pid == monitored_pid)
+		ima_hash_ret = bpf_ima_inode_hash(bprm->file->f_inode,
+						  &ima_hash, sizeof(ima_hash));
+
+	return 0;
+}