| Message ID | 20201121005054.3467947-3-kpsingh@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2,1/3] ima: Implement ima_inode_hash | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | warning | Series does not have a cover letter |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | fail | ERROR: do not initialise globals to 0 |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Sat, 2020-11-21 at 00:50 +0000, KP Singh wrote: > From: KP Singh <kpsingh@google.com> > > - Update the IMA policy before executing the test binary (this is not an > override of the policy, just an append that ensures that hashes are > calculated on executions). Assuming the builtin policy has been replaced with a custom policy and CONFIG_IMA_WRITE_POLICY is enabled, then yes the rule is appended. If a custom policy has not yet been loaded, loading this rule becomes the defacto custom policy. Even if a custom policy has been loaded, potentially additional measurements unrelated to this test would be included the measurement list. One way of limiting a rule to a specific test is by loopback mounting a file system and defining a policy rule based on the loopback mount unique uuid. Mimi
On Mon, Nov 23, 2020 at 2:24 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Sat, 2020-11-21 at 00:50 +0000, KP Singh wrote: > > From: KP Singh <kpsingh@google.com> > > > > - Update the IMA policy before executing the test binary (this is not an > > override of the policy, just an append that ensures that hashes are > > calculated on executions). > > Assuming the builtin policy has been replaced with a custom policy and > CONFIG_IMA_WRITE_POLICY is enabled, then yes the rule is appended. If > a custom policy has not yet been loaded, loading this rule becomes the > defacto custom policy. > > Even if a custom policy has been loaded, potentially additional > measurements unrelated to this test would be included the measurement > list. One way of limiting a rule to a specific test is by loopback > mounting a file system and defining a policy rule based on the loopback > mount unique uuid. Thanks Mimi! I wonder if we simply limit this to policy to /tmp and run an executable from /tmp (like test_local_storage.c does). The only side effect would be of extra hashes being calculated on binaries run from /tmp which is not too bad I guess? We could do the loop mount too, but I am guessing the most clean way would be to shell out to mount from the test? Are there some other examples of IMA we could look at? - KP > > Mimi >
[Cc'ing Petr Vorel] On Mon, 2020-11-23 at 15:06 +0100, KP Singh wrote: > On Mon, Nov 23, 2020 at 2:24 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Sat, 2020-11-21 at 00:50 +0000, KP Singh wrote: > > > From: KP Singh <kpsingh@google.com> > > > > > > - Update the IMA policy before executing the test binary (this is not an > > > override of the policy, just an append that ensures that hashes are > > > calculated on executions). > > > > Assuming the builtin policy has been replaced with a custom policy and > > CONFIG_IMA_WRITE_POLICY is enabled, then yes the rule is appended. If > > a custom policy has not yet been loaded, loading this rule becomes the > > defacto custom policy. > > > > Even if a custom policy has been loaded, potentially additional > > measurements unrelated to this test would be included the measurement > > list. One way of limiting a rule to a specific test is by loopback > > mounting a file system and defining a policy rule based on the loopback > > mount unique uuid. > > Thanks Mimi! > > I wonder if we simply limit this to policy to /tmp and run an executable > from /tmp (like test_local_storage.c does). > > The only side effect would be of extra hashes being calculated on > binaries run from /tmp which is not too bad I guess? The builtin measurement policy (ima_policy=tcb") explicitly defines a rule to not measure /tmp files. Measuring /tmp results in a lot of measurements. {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC}, > > We could do the loop mount too, but I am guessing the most clean way > would be to shell out to mount from the test? Are there some other examples > of IMA we could look at? LTP loopback mounts a filesystem, since /tmp is not being measured with the builtin "tcb" policy. Defining new policy rules should be limited to the loopback mount. This would pave the way for defining IMA- appraisal signature verification policy rules, without impacting the running system. Mimi
[...] > > > > > > Even if a custom policy has been loaded, potentially additional > > > measurements unrelated to this test would be included the measurement > > > list. One way of limiting a rule to a specific test is by loopback > > > mounting a file system and defining a policy rule based on the loopback > > > mount unique uuid. > > > > Thanks Mimi! > > > > I wonder if we simply limit this to policy to /tmp and run an executable > > from /tmp (like test_local_storage.c does). > > > > The only side effect would be of extra hashes being calculated on > > binaries run from /tmp which is not too bad I guess? > > The builtin measurement policy (ima_policy=tcb") explicitly defines a > rule to not measure /tmp files. Measuring /tmp results in a lot of > measurements. > > {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC}, > > > > > We could do the loop mount too, but I am guessing the most clean way > > would be to shell out to mount from the test? Are there some other examples > > of IMA we could look at? > > LTP loopback mounts a filesystem, since /tmp is not being measured with > the builtin "tcb" policy. Defining new policy rules should be limited > to the loopback mount. This would pave the way for defining IMA- > appraisal signature verification policy rules, without impacting the > running system. +Andrii Do you think we can split the IMA test out, have a little shell script that does the loopback mount, gets the FS UUID, updates the IMA policy and then runs a C program? This would also allow "test_progs" to be independent of CONFIG_IMA. I am guessing the structure would be something similar to test_xdp_redirect.sh - KP > > Mimi >
On 11/23/20 10:27 AM, KP Singh wrote: > [...] > >>>> >>>> Even if a custom policy has been loaded, potentially additional >>>> measurements unrelated to this test would be included the measurement >>>> list. One way of limiting a rule to a specific test is by loopback >>>> mounting a file system and defining a policy rule based on the loopback >>>> mount unique uuid. >>> >>> Thanks Mimi! >>> >>> I wonder if we simply limit this to policy to /tmp and run an executable >>> from /tmp (like test_local_storage.c does). >>> >>> The only side effect would be of extra hashes being calculated on >>> binaries run from /tmp which is not too bad I guess? >> >> The builtin measurement policy (ima_policy=tcb") explicitly defines a >> rule to not measure /tmp files. Measuring /tmp results in a lot of >> measurements. >> >> {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC}, >> >>> >>> We could do the loop mount too, but I am guessing the most clean way >>> would be to shell out to mount from the test? Are there some other examples >>> of IMA we could look at? >> >> LTP loopback mounts a filesystem, since /tmp is not being measured with >> the builtin "tcb" policy. Defining new policy rules should be limited >> to the loopback mount. This would pave the way for defining IMA- >> appraisal signature verification policy rules, without impacting the >> running system. > > +Andrii > > Do you think we can split the IMA test out, > have a little shell script that does the loopback mount, gets the > FS UUID, updates the IMA policy and then runs a C program? > > This would also allow "test_progs" to be independent of CONFIG_IMA. > > I am guessing the structure would be something similar > to test_xdp_redirect.sh Look at sk_assign test. sk_assign.c: if (CHECK_FAIL(system("ip link set dev lo up"))) sk_assign.c: if (CHECK_FAIL(system("ip route add local default dev lo"))) sk_assign.c: if (CHECK_FAIL(system("ip -6 route add local default dev lo"))) sk_assign.c: if (CHECK_FAIL(system("tc qdisc add dev lo clsact"))) sk_assign.c: if (CHECK(system(tc_cmd), "BPF load failed;" You can use "system" to invoke some bash commands to simulate a script in the tests. > > - KP > >> >> Mimi >>
On Mon, Nov 23, 2020 at 7:36 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 11/23/20 10:27 AM, KP Singh wrote: > > [...] > > > >>>> > >>>> Even if a custom policy has been loaded, potentially additional > >>>> measurements unrelated to this test would be included the measurement > >>>> list. One way of limiting a rule to a specific test is by loopback > >>>> mounting a file system and defining a policy rule based on the loopback > >>>> mount unique uuid. > >>> > >>> Thanks Mimi! > >>> > >>> I wonder if we simply limit this to policy to /tmp and run an executable > >>> from /tmp (like test_local_storage.c does). > >>> > >>> The only side effect would be of extra hashes being calculated on > >>> binaries run from /tmp which is not too bad I guess? > >> > >> The builtin measurement policy (ima_policy=tcb") explicitly defines a > >> rule to not measure /tmp files. Measuring /tmp results in a lot of > >> measurements. > >> > >> {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC}, > >> > >>> > >>> We could do the loop mount too, but I am guessing the most clean way > >>> would be to shell out to mount from the test? Are there some other examples > >>> of IMA we could look at? > >> > >> LTP loopback mounts a filesystem, since /tmp is not being measured with > >> the builtin "tcb" policy. Defining new policy rules should be limited > >> to the loopback mount. This would pave the way for defining IMA- > >> appraisal signature verification policy rules, without impacting the > >> running system. > > > > +Andrii > > > > Do you think we can split the IMA test out, > > have a little shell script that does the loopback mount, gets the > > FS UUID, updates the IMA policy and then runs a C program? > > > > This would also allow "test_progs" to be independent of CONFIG_IMA. > > > > I am guessing the structure would be something similar > > to test_xdp_redirect.sh > > Look at sk_assign test. > > sk_assign.c: if (CHECK_FAIL(system("ip link set dev lo up"))) > sk_assign.c: if (CHECK_FAIL(system("ip route add local default dev lo"))) > sk_assign.c: if (CHECK_FAIL(system("ip -6 route add local default dev > lo"))) > sk_assign.c: if (CHECK_FAIL(system("tc qdisc add dev lo clsact"))) > sk_assign.c: if (CHECK(system(tc_cmd), "BPF load failed;" > > You can use "system" to invoke some bash commands to simulate a script > in the tests. Heh, that's what I was trying to avoid, I need to parse the output to the get the name of which loop device was assigned and then call a command like: # blkid /dev/loop0 /dev/loop0: UUID="607ed7ce-3fad-4236-8faf-8ab744f23e01" TYPE="ext3" Running simple commands with "system" seems okay but parsing output is a bit too much :) I read about: https://man7.org/linux/man-pages/man4/loop.4.html But I still need to create a backing file, format it and then get the UUID. Any simple trick that I may be missing? - KP > > > > > - KP > > > >> > >> Mimi > >>
On 11/23/20 10:46 AM, KP Singh wrote: > On Mon, Nov 23, 2020 at 7:36 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 11/23/20 10:27 AM, KP Singh wrote: >>> [...] >>> >>>>>> >>>>>> Even if a custom policy has been loaded, potentially additional >>>>>> measurements unrelated to this test would be included the measurement >>>>>> list. One way of limiting a rule to a specific test is by loopback >>>>>> mounting a file system and defining a policy rule based on the loopback >>>>>> mount unique uuid. >>>>> >>>>> Thanks Mimi! >>>>> >>>>> I wonder if we simply limit this to policy to /tmp and run an executable >>>>> from /tmp (like test_local_storage.c does). >>>>> >>>>> The only side effect would be of extra hashes being calculated on >>>>> binaries run from /tmp which is not too bad I guess? >>>> >>>> The builtin measurement policy (ima_policy=tcb") explicitly defines a >>>> rule to not measure /tmp files. Measuring /tmp results in a lot of >>>> measurements. >>>> >>>> {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC}, >>>> >>>>> >>>>> We could do the loop mount too, but I am guessing the most clean way >>>>> would be to shell out to mount from the test? Are there some other examples >>>>> of IMA we could look at? >>>> >>>> LTP loopback mounts a filesystem, since /tmp is not being measured with >>>> the builtin "tcb" policy. Defining new policy rules should be limited >>>> to the loopback mount. This would pave the way for defining IMA- >>>> appraisal signature verification policy rules, without impacting the >>>> running system. >>> >>> +Andrii >>> >>> Do you think we can split the IMA test out, >>> have a little shell script that does the loopback mount, gets the >>> FS UUID, updates the IMA policy and then runs a C program? >>> >>> This would also allow "test_progs" to be independent of CONFIG_IMA. >>> >>> I am guessing the structure would be something similar >>> to test_xdp_redirect.sh >> >> Look at sk_assign test. >> >> sk_assign.c: if (CHECK_FAIL(system("ip link set dev lo up"))) >> sk_assign.c: if (CHECK_FAIL(system("ip route add local default dev lo"))) >> sk_assign.c: if (CHECK_FAIL(system("ip -6 route add local default dev >> lo"))) >> sk_assign.c: if (CHECK_FAIL(system("tc qdisc add dev lo clsact"))) >> sk_assign.c: if (CHECK(system(tc_cmd), "BPF load failed;" >> >> You can use "system" to invoke some bash commands to simulate a script >> in the tests. > > Heh, that's what I was trying to avoid, I need to parse the output to the get > the name of which loop device was assigned and then call a command like: > > # blkid /dev/loop0 > /dev/loop0: UUID="607ed7ce-3fad-4236-8faf-8ab744f23e01" TYPE="ext3" > > Running simple commands with "system" seems okay but parsing output > is a bit too much :) > > I read about: > > https://man7.org/linux/man-pages/man4/loop.4.html > > But I still need to create a backing file, format it and then get the UUID. > > Any simple trick that I may be missing? Maybe you can create a bash script on your prog_test files and do system("./<>.sh"). In the shell script, you can use all the bash magic (sed, awk, etc) to parse and store the needed result in a temp file, and after a successful system(""), you just read that temp file. Does this work? > - KP > >> >>> >>> - KP >>> >>>> >>>> Mimi >>>>
On 11/23/20 10:54 AM, Yonghong Song wrote: > > > On 11/23/20 10:46 AM, KP Singh wrote: >> On Mon, Nov 23, 2020 at 7:36 PM Yonghong Song <yhs@fb.com> wrote: >>> >>> >>> >>> On 11/23/20 10:27 AM, KP Singh wrote: >>>> [...] >>>> >>>>>>> >>>>>>> Even if a custom policy has been loaded, potentially additional >>>>>>> measurements unrelated to this test would be included the >>>>>>> measurement >>>>>>> list. One way of limiting a rule to a specific test is by loopback >>>>>>> mounting a file system and defining a policy rule based on the >>>>>>> loopback >>>>>>> mount unique uuid. >>>>>> >>>>>> Thanks Mimi! >>>>>> >>>>>> I wonder if we simply limit this to policy to /tmp and run an >>>>>> executable >>>>>> from /tmp (like test_local_storage.c does). >>>>>> >>>>>> The only side effect would be of extra hashes being calculated on >>>>>> binaries run from /tmp which is not too bad I guess? >>>>> >>>>> The builtin measurement policy (ima_policy=tcb") explicitly defines a >>>>> rule to not measure /tmp files. Measuring /tmp results in a lot of >>>>> measurements. >>>>> >>>>> {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = >>>>> IMA_FSMAGIC}, >>>>> >>>>>> >>>>>> We could do the loop mount too, but I am guessing the most clean way >>>>>> would be to shell out to mount from the test? Are there some other >>>>>> examples >>>>>> of IMA we could look at? >>>>> >>>>> LTP loopback mounts a filesystem, since /tmp is not being measured >>>>> with >>>>> the builtin "tcb" policy. Defining new policy rules should be limited >>>>> to the loopback mount. This would pave the way for defining IMA- >>>>> appraisal signature verification policy rules, without impacting the >>>>> running system. >>>> >>>> +Andrii >>>> >>>> Do you think we can split the IMA test out, >>>> have a little shell script that does the loopback mount, gets the >>>> FS UUID, updates the IMA policy and then runs a C program? >>>> >>>> This would also allow "test_progs" to be independent of CONFIG_IMA. >>>> >>>> I am guessing the structure would be something similar >>>> to test_xdp_redirect.sh >>> >>> Look at sk_assign test. >>> >>> sk_assign.c: if (CHECK_FAIL(system("ip link set dev lo up"))) >>> sk_assign.c: if (CHECK_FAIL(system("ip route add local default dev >>> lo"))) >>> sk_assign.c: if (CHECK_FAIL(system("ip -6 route add local default dev >>> lo"))) >>> sk_assign.c: if (CHECK_FAIL(system("tc qdisc add dev lo clsact"))) >>> sk_assign.c: if (CHECK(system(tc_cmd), "BPF load failed;" >>> >>> You can use "system" to invoke some bash commands to simulate a script >>> in the tests. >> >> Heh, that's what I was trying to avoid, I need to parse the output to >> the get >> the name of which loop device was assigned and then call a command like: >> >> # blkid /dev/loop0 >> /dev/loop0: UUID="607ed7ce-3fad-4236-8faf-8ab744f23e01" TYPE="ext3" >> >> Running simple commands with "system" seems okay but parsing output >> is a bit too much :) >> >> I read about: >> >> https://man7.org/linux/man-pages/man4/loop.4.html >> >> But I still need to create a backing file, format it and then get the >> UUID. >> >> Any simple trick that I may be missing? > > Maybe you can create a bash script on your prog_test files and do > system("./<>.sh"). In the shell script, you can use all the bash magic > (sed, awk, etc) to parse and store the needed result in a temp file, and > after a successful system(""), you just read that temp file. Does this > work? I guess under the current framework, you can also create a .sh file manually and place it into tools/testing/selftests/bpf directory and call it in your prog_tests .c file with system("./<>.sh")... > >> - KP >> >>> >>>> >>>> - KP >>>> >>>>> >>>>> Mimi >>>>>
diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config index 2118e23ac07a..4b5764031368 100644 --- a/tools/testing/selftests/bpf/config +++ b/tools/testing/selftests/bpf/config @@ -39,3 +39,6 @@ CONFIG_BPF_JIT=y CONFIG_BPF_LSM=y CONFIG_SECURITY=y CONFIG_LIRC=y +CONFIG_IMA=y +CONFIG_IMA_WRITE_POLICY=y +CONFIG_IMA_READ_POLICY=y diff --git a/tools/testing/selftests/bpf/prog_tests/test_lsm.c b/tools/testing/selftests/bpf/prog_tests/test_lsm.c index 6ab29226c99b..bcb050a296a4 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_lsm.c +++ b/tools/testing/selftests/bpf/prog_tests/test_lsm.c @@ -52,6 +52,28 @@ int exec_cmd(int *monitored_pid) return -EINVAL; } +#define IMA_POLICY "measure func=BPRM_CHECK" + +/* This does not override the policy, IMA policy updates are + * append only, so this just ensures that "measure func=BPRM_CHECK" + * is in the policy. IMA does not allow us to remove this line once + * it is added. + */ +static int update_ima_policy(void) +{ + int fd, ret = 0; + + fd = open("/sys/kernel/security/ima/policy", O_WRONLY); + if (fd < 0) + return -errno; + + if (write(fd, IMA_POLICY, sizeof(IMA_POLICY)) == -1) + ret = -errno; + + close(fd); + return ret; +} + void test_test_lsm(void) { struct lsm *skel = NULL; @@ -66,6 +88,10 @@ void test_test_lsm(void) if (CHECK(err, "attach", "lsm attach failed: %d\n", err)) goto close_prog; + err = update_ima_policy(); + if (CHECK(err, "update_ima_policy", "err %d\n", err)) + goto close_prog; + err = exec_cmd(&skel->bss->monitored_pid); if (CHECK(err < 0, "exec_cmd", "err %d errno %d\n", err, errno)) goto close_prog; @@ -83,6 +109,12 @@ void test_test_lsm(void) CHECK(skel->bss->mprotect_count != 1, "mprotect_count", "mprotect_count = %d\n", skel->bss->mprotect_count); + 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); + syscall(__NR_setdomainname, &buf, -2L); syscall(__NR_setdomainname, 0, -3L); syscall(__NR_setdomainname, ~0L, -4L); diff --git a/tools/testing/selftests/bpf/progs/lsm.c b/tools/testing/selftests/bpf/progs/lsm.c index ff4d343b94b5..5adc193e414d 100644 --- a/tools/testing/selftests/bpf/progs/lsm.c +++ b/tools/testing/selftests/bpf/progs/lsm.c @@ -35,6 +35,8 @@ char _license[] SEC("license") = "GPL"; int monitored_pid = 0; int mprotect_count = 0; int bprm_count = 0; +long ima_hash_ret = -1; +u64 ima_hash = 0; SEC("lsm/file_mprotect") int BPF_PROG(test_int_hook, struct vm_area_struct *vma, @@ -65,8 +67,11 @@ int BPF_PROG(test_void_hook, struct linux_binprm *bprm) __u32 key = 0; __u64 *value; - if (monitored_pid == pid) + if (monitored_pid == pid) { bprm_count++; + ima_hash_ret = bpf_ima_inode_hash(bprm->file->f_inode, + &ima_hash, sizeof(ima_hash)); + } bpf_copy_from_user(args, sizeof(args), (void *)bprm->vma->vm_mm->arg_start); bpf_copy_from_user(args, sizeof(args), (void *)bprm->mm->arg_start);