LinuxLists.cc - [PATCH bpf-next v3 3/3] bpf: Add a selftest for bpf_ima_inode

2020-11-24 15:15:56

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

From: KP Singh <[email protected]>

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 <[email protected]>
---
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;
+}
--
2.29.2.454.gaff20da3a2-goog

2020-11-24 18:10:53

by Yonghong Song

[permalink] [raw]

Subject: Re: [PATCH bpf-next v3 3/3] bpf: Add a selftest for bpf_ima_inode_hash

2020-11-25 02:25:04

by Mimi Zohar

[permalink] [raw]

Subject: Re: [PATCH bpf-next v3 3/3] bpf: Add a selftest for bpf_ima_inode_hash

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

2020-11-25 02:59:51

by KP Singh

[permalink] [raw]

Subject: Re: [PATCH bpf-next v3 3/3] bpf: Add a selftest for bpf_ima_inode_hash

On Wed, Nov 25, 2020 at 3:20 AM Mimi Zohar <[email protected]> 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
>

2020-11-25 03:04:27

by Mimi Zohar

[permalink] [raw]

Subject: Re: [PATCH bpf-next v3 3/3] bpf: Add a selftest for bpf_ima_inode_hash

On Wed, 2020-11-25 at 03:55 +0100, KP Singh wrote:
> On Wed, Nov 25, 2020 at 3:20 AM Mimi Zohar <[email protected]> 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.

2020-11-25 12:30:03

by Mimi Zohar

[permalink] [raw]

Subject: Re: [PATCH bpf-next v3 3/3] bpf: Add a selftest for bpf_ima_inode_hash

On Tue, 2020-11-24 at 15:12 +0000, KP Singh wrote:
> From: KP Singh <[email protected]>
>
> 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 <[email protected]>

Suggested-by: Mimi Zohar <[email protected]> (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;
> +}

2020-11-26 09:52:51

by Yonghong Song

[permalink] [raw]

Subject: Re: [PATCH bpf-next v3 3/3] bpf: Add a selftest for bpf_ima_inode_hash

On 11/24/20 7:12 AM, KP Singh wrote:
> From: KP Singh <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
> +}
> +
[...]

2020-11-27 08:33:36

by KP Singh

[permalink] [raw]

Subject: Re: [PATCH bpf-next v3 3/3] bpf: Add a selftest for bpf_ima_inode_hash

[...]

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

2020-11-27 08:50:00

by Andrii Nakryiko

[permalink] [raw]

Subject: Re: [PATCH bpf-next v3 3/3] bpf: Add a selftest for bpf_ima_inode_hash

On Tue, Nov 24, 2020 at 7:16 AM KP Singh <[email protected]> wrote:
>
> From: KP Singh <[email protected]>
>
> 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 <[email protected]>
> ---
> 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}
> +}
> +

[...]

2020-11-27 13:15:15

by KP Singh

[permalink] [raw]

Subject: Re: [PATCH bpf-next v3 3/3] bpf: Add a selftest for bpf_ima_inode_hash

On Fri, Nov 27, 2020 at 5:29 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Tue, Nov 24, 2020 at 7:16 AM KP Singh <[email protected]> wrote:
> >
> > From: KP Singh <[email protected]>
> >

[...]

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

[...]