| Message ID | 20240711-landlock-v3-5-c7b0e9edf9b0@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | landlock testing suite | expand |
Hi Andrea, again, LGTM, 2 things: First, this fails at least on various kernel versions (tested: Tumbleweed 6.10.0-rc7-3.g92abc10-default, and SLE15-SP4 5.14.21 with lsm=landlock and Debian 6.6.15-amd64): landlock01.c:49: TFAIL: Size is too small expected EINVAL: ENOMSG (42) Is it a kernel bug or a test bug? I'm not sure if Li's concern [1] was fully addressed: We still have to adjust the case if someone introduces one more new field similar to 'handled_access_net' to the structure in the future. Kind regards, Petr [1] https://lore.kernel.org/ltp/CAEemH2dkKvthbx+za-rwfsmanraZuud-sq1O4FZK2zta5MBMSg@mail.gmail.com/
Hi Petr, On Fri, Jul 12, 2024 at 4:40 AM Petr Vorel <pvorel@suse.cz> wrote: > Hi Andrea, > > again, LGTM, 2 things: > > First, this fails at least on various kernel versions (tested: Tumbleweed > 6.10.0-rc7-3.g92abc10-default, and SLE15-SP4 5.14.21 with lsm=landlock and > Debian 6.6.15-amd64): > > landlock01.c:49: TFAIL: Size is too small expected EINVAL: ENOMSG (42) > > Is it a kernel bug or a test bug? > You probably need to check the `/usr/include/linux/landlock.h` header file exist, and to see if 'struct landlock_ruleset_attr' contains the new field 'handled_access_net'. If not exist or does not contain that, the test defines 'struct landlock_ruleset_attr' in lapi/landlock.h which contains handled_access_net directly, this is likely the root cause lead test failed on your box. > > I'm not sure if Li's concern [1] was fully addressed: > > We still have to adjust the case if someone introduces one more > new field > similar to 'handled_access_net' to the structure in the future. > > Kind regards, > Petr > > [1] > https://lore.kernel.org/ltp/CAEemH2dkKvthbx+za-rwfsmanraZuud-sq1O4FZK2zta5MBMSg@mail.gmail.com/ > >
On Fri, Jul 12, 2024 at 10:11 AM Li Wang <liwang@redhat.com> wrote: > Hi Petr, > > On Fri, Jul 12, 2024 at 4:40 AM Petr Vorel <pvorel@suse.cz> wrote: > >> Hi Andrea, >> >> again, LGTM, 2 things: >> >> First, this fails at least on various kernel versions (tested: Tumbleweed >> 6.10.0-rc7-3.g92abc10-default, and SLE15-SP4 5.14.21 with lsm=landlock and >> Debian 6.6.15-amd64): >> >> landlock01.c:49: TFAIL: Size is too small expected EINVAL: ENOMSG (42) >> >> Is it a kernel bug or a test bug? >> > > You probably need to check the `/usr/include/linux/landlock.h` header file > exist, > and to see if 'struct landlock_ruleset_attr' contains the new field > 'handled_access_net'. > > If not exist or does not contain that, the test defines 'struct > landlock_ruleset_attr' > in lapi/landlock.h which contains handled_access_net directly, this is > likely the > root cause lead test failed on your box. > And, if the header file does not exist, the macro condition will choose to use 'rule_size - 1', and that caused the ENOMSG error during test on the newer kernel. #ifdef HAVE_STRUCT_LANDLOCK_RULESET_ATTR_HANDLED_ACCESS_NET rule_small_size = rule_size - sizeof(uint64_t) - 1; #else rule_small_size = rule_size - 1; #endif So to keep the kernel-headers and running kernel version consistent should be required for the landlock01 test. Otherwise the #ifdef possibly won't work correctly. I guess we might have to resolve this on the test side. > > > >> >> I'm not sure if Li's concern [1] was fully addressed: >> >> We still have to adjust the case if someone introduces one more >> new field >> similar to 'handled_access_net' to the structure in the future. >> >> Kind regards, >> Petr >> >> [1] >> https://lore.kernel.org/ltp/CAEemH2dkKvthbx+za-rwfsmanraZuud-sq1O4FZK2zta5MBMSg@mail.gmail.com/ >> >> > > -- > Regards, > Li Wang >
Hi Li, Andrea, > Hi Petr, > On Fri, Jul 12, 2024 at 4:40 AM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Andrea, > > again, LGTM, 2 things: > > First, this fails at least on various kernel versions (tested: Tumbleweed > > 6.10.0-rc7-3.g92abc10-default, and SLE15-SP4 5.14.21 with lsm=landlock and > > Debian 6.6.15-amd64): > > landlock01.c:49: TFAIL: Size is too small expected EINVAL: ENOMSG (42) > > Is it a kernel bug or a test bug? > You probably need to check the `/usr/include/linux/landlock.h` header file > exist, > and to see if 'struct landlock_ruleset_attr' contains the new field > 'handled_access_net'. > If not exist or does not contain that, the test defines 'struct > landlock_ruleset_attr' > in lapi/landlock.h which contains handled_access_net directly, this is > likely the > root cause lead test failed on your box. I'm sorry for a noise, my bad. Basic error - I forget to run make autotools && ./configure Thanks, patch merged. Kind regards, Petr
Hi Andrea, Li, ... > >> First, this fails at least on various kernel versions (tested: Tumbleweed > >> 6.10.0-rc7-3.g92abc10-default, and SLE15-SP4 5.14.21 with lsm=landlock and > >> Debian 6.6.15-amd64): > >> landlock01.c:49: TFAIL: Size is too small expected EINVAL: ENOMSG (42) > >> Is it a kernel bug or a test bug? > > You probably need to check the `/usr/include/linux/landlock.h` header file > > exist, > > and to see if 'struct landlock_ruleset_attr' contains the new field > > 'handled_access_net'. > > If not exist or does not contain that, the test defines 'struct > > landlock_ruleset_attr' > > in lapi/landlock.h which contains handled_access_net directly, this is > > likely the > > root cause lead test failed on your box. > And, if the header file does not exist, the macro condition will choose to > use 'rule_size - 1', > and that caused the ENOMSG error during test on the newer kernel. > #ifdef HAVE_STRUCT_LANDLOCK_RULESET_ATTR_HANDLED_ACCESS_NET > rule_small_size = rule_size - sizeof(uint64_t) - 1; > #else > rule_small_size = rule_size - 1; > #endif > So to keep the kernel-headers and running kernel version consistent should > be required > for the landlock01 test. Otherwise the #ifdef possibly won't work correctly. FYI Having inconsistent kernel headers and running kernel would be a problem for more LTP tests than just landlock01 (basically many tests which have autotools check). But this can be problematic for some development (e.g. linux-next). Therefore we at least assume UAPI headers shouldn't be newer than running kernel, see https://lore.kernel.org/ltp/ZJP_qPeJ37H4qhEN@yuki/. > I guess we might have to resolve this on the test side. Trying to compare versions <linux/version.h> could be used: #define LINUX_VERSION_CODE 395008 #define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + ((c) > 255 ? 255 : (c))) #define LINUX_VERSION_MAJOR 6 #define LINUX_VERSION_PATCHLEVEL 7 #define LINUX_VERSION_SUBLEVEL 0 We already use KERNEL_VERSION() in kdump and device-drivers tests. Kind regards, Petr
Hi Petr, On Fri, Jul 12, 2024 at 3:58 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Andrea, Li, > > ... > > >> First, this fails at least on various kernel versions (tested: > Tumbleweed > > >> 6.10.0-rc7-3.g92abc10-default, and SLE15-SP4 5.14.21 with > lsm=landlock and > > >> Debian 6.6.15-amd64): > > > >> landlock01.c:49: TFAIL: Size is too small expected EINVAL: ENOMSG (42) > > > >> Is it a kernel bug or a test bug? > > > > > You probably need to check the `/usr/include/linux/landlock.h` header > file > > > exist, > > > and to see if 'struct landlock_ruleset_attr' contains the new field > > > 'handled_access_net'. > > > > If not exist or does not contain that, the test defines 'struct > > > landlock_ruleset_attr' > > > in lapi/landlock.h which contains handled_access_net directly, this is > > > likely the > > > root cause lead test failed on your box. > > > And, if the header file does not exist, the macro condition will choose > to > > use 'rule_size - 1', > > and that caused the ENOMSG error during test on the newer kernel. > > > #ifdef HAVE_STRUCT_LANDLOCK_RULESET_ATTR_HANDLED_ACCESS_NET > > rule_small_size = rule_size - sizeof(uint64_t) - 1; > > #else > > rule_small_size = rule_size - 1; > > #endif > > > So to keep the kernel-headers and running kernel version consistent > should > > be required > > for the landlock01 test. Otherwise the #ifdef possibly won't work > correctly. > > FYI Having inconsistent kernel headers and running kernel would be a > problem for > more LTP tests than just landlock01 (basically many tests which have > autotools > check). But this can be problematic for some development (e.g. linux-next). > Therefore we at least assume UAPI headers shouldn't be newer than running > kernel, see https://lore.kernel.org/ltp/ZJP_qPeJ37H4qhEN@yuki/. > Yes, I agree on this. As landlock01 uses the macro I pointed out in the last email, it is almost unable to set a correct rule_small_size w/o variants of kernel-headers. So I still think just simply set the 'rule_small_size' to 'sizeof(__u64) - 1;' will make life easier but Andrea has a different perspective on that. Anyway, I would leave this to Andrea (an excellent black-box tester) for more struggling. lol~ > > I guess we might have to resolve this on the test side. > > Trying to compare versions <linux/version.h> could be used: > > #define LINUX_VERSION_CODE 395008 > #define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + ((c) > 255 ? 255 > : (c))) > #define LINUX_VERSION_MAJOR 6 > #define LINUX_VERSION_PATCHLEVEL 7 > #define LINUX_VERSION_SUBLEVEL 0 > > We already use KERNEL_VERSION() in kdump and device-drivers tests. > It could work by adding more kernel judgment for the macro definition, but a little bit of a mess IMHO.
> Hi Petr, > On Fri, Jul 12, 2024 at 3:58 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Andrea, Li, > > ... > > > >> First, this fails at least on various kernel versions (tested: > > Tumbleweed > > > >> 6.10.0-rc7-3.g92abc10-default, and SLE15-SP4 5.14.21 with > > lsm=landlock and > > > >> Debian 6.6.15-amd64): > > > >> landlock01.c:49: TFAIL: Size is too small expected EINVAL: ENOMSG (42) > > > >> Is it a kernel bug or a test bug? > > > > You probably need to check the `/usr/include/linux/landlock.h` header > > file > > > > exist, > > > > and to see if 'struct landlock_ruleset_attr' contains the new field > > > > 'handled_access_net'. > > > > If not exist or does not contain that, the test defines 'struct > > > > landlock_ruleset_attr' > > > > in lapi/landlock.h which contains handled_access_net directly, this is > > > > likely the > > > > root cause lead test failed on your box. > > > And, if the header file does not exist, the macro condition will choose > > to > > > use 'rule_size - 1', > > > and that caused the ENOMSG error during test on the newer kernel. > > > #ifdef HAVE_STRUCT_LANDLOCK_RULESET_ATTR_HANDLED_ACCESS_NET > > > rule_small_size = rule_size - sizeof(uint64_t) - 1; > > > #else > > > rule_small_size = rule_size - 1; > > > #endif > > > So to keep the kernel-headers and running kernel version consistent > > should > > > be required > > > for the landlock01 test. Otherwise the #ifdef possibly won't work > > correctly. > > FYI Having inconsistent kernel headers and running kernel would be a > > problem for > > more LTP tests than just landlock01 (basically many tests which have > > autotools > > check). But this can be problematic for some development (e.g. linux-next). > > Therefore we at least assume UAPI headers shouldn't be newer than running > > kernel, see https://lore.kernel.org/ltp/ZJP_qPeJ37H4qhEN@yuki/. > Yes, I agree on this. > As landlock01 uses the macro I pointed out in the last email, > it is almost unable to set a correct rule_small_size w/o > variants of kernel-headers. > So I still think just simply set the 'rule_small_size' to 'sizeof(__u64) - > 1;' > will make life easier but Andrea has a different perspective on that. It looks to me also better, but let's ask others :). @Jan, @Cyril WDYT? > Anyway, I would leave this to Andrea (an excellent black-box tester) > for more struggling. lol~ Lol :). I suppose userspace developers which use raw syscalls are often forced to look into kernel sources (man pages are sparse). Kind regards, Petr > > > I guess we might have to resolve this on the test side. > > Trying to compare versions <linux/version.h> could be used: > > #define LINUX_VERSION_CODE 395008 > > #define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + ((c) > 255 ? 255 > > : (c))) > > #define LINUX_VERSION_MAJOR 6 > > #define LINUX_VERSION_PATCHLEVEL 7 > > #define LINUX_VERSION_SUBLEVEL 0 > > We already use KERNEL_VERSION() in kdump and device-drivers tests. > It could work by adding more kernel judgment for the macro definition, > but a little bit of a mess IMHO.
diff --git a/runtest/syscalls b/runtest/syscalls index a7cf296a9..d0a9bd14e 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -685,6 +685,8 @@ kill11 kill11 kill12 kill12 kill13 kill13 +landlock01 landlock01 + lchown01 lchown01 lchown01_16 lchown01_16 lchown02 lchown02 diff --git a/testcases/kernel/syscalls/landlock/.gitignore b/testcases/kernel/syscalls/landlock/.gitignore new file mode 100644 index 000000000..b69f9b94a --- /dev/null +++ b/testcases/kernel/syscalls/landlock/.gitignore @@ -0,0 +1 @@ +landlock01 diff --git a/testcases/kernel/syscalls/landlock/Makefile b/testcases/kernel/syscalls/landlock/Makefile new file mode 100644 index 000000000..8cf1b9024 --- /dev/null +++ b/testcases/kernel/syscalls/landlock/Makefile @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com> + +top_srcdir ?= ../../../.. + +include $(top_srcdir)/include/mk/testcases.mk +include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/syscalls/landlock/landlock01.c b/testcases/kernel/syscalls/landlock/landlock01.c new file mode 100644 index 000000000..0c50b55d8 --- /dev/null +++ b/testcases/kernel/syscalls/landlock/landlock01.c @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com> + */ + +/*\ + * [Description] + * + * This test verifies that landlock_create_ruleset syscall fails with the right + * error codes: + * + * - EINVAL Unknown flags, or unknown access, or too small size + * - E2BIG size is too big + * - EFAULT attr was not a valid address + * - ENOMSG Empty accesses (i.e., attr->handled_access_fs is 0) + */ + +#include "landlock_common.h" + +static struct landlock_ruleset_attr *ruleset_attr; +static struct landlock_ruleset_attr *null_attr; +static size_t rule_size; +static size_t rule_small_size; +static size_t rule_big_size; + +static struct tcase { + struct landlock_ruleset_attr **attr; + uint64_t access_fs; + size_t *size; + uint32_t flags; + int exp_errno; + char *msg; +} tcases[] = { + {&ruleset_attr, -1, &rule_size, 0, EINVAL, "Unknown access"}, + {&ruleset_attr, 0, &rule_small_size, 0, EINVAL, "Size is too small"}, + {&ruleset_attr, 0, &rule_size, -1, EINVAL, "Unknown flags"}, + {&ruleset_attr, 0, &rule_big_size, 0, E2BIG, "Size is too big"}, + {&null_attr, 0, &rule_size, 0, EFAULT, "Invalid attr address"}, + {&ruleset_attr, 0, &rule_size, 0, ENOMSG, "Empty accesses"}, +}; + +static void run(unsigned int n) +{ + struct tcase *tc = &tcases[n]; + + if (*tc->attr) + (*tc->attr)->handled_access_fs = tc->access_fs; + + TST_EXP_FAIL(tst_syscall(__NR_landlock_create_ruleset, + *tc->attr, *tc->size, tc->flags), + tc->exp_errno, + "%s", + tc->msg); + + if (TST_RET >= 0) + SAFE_CLOSE(TST_RET); +} + +static void setup(void) +{ + verify_landlock_is_enabled(); + + rule_size = sizeof(struct landlock_ruleset_attr); + +#ifdef HAVE_STRUCT_LANDLOCK_RULESET_ATTR_HANDLED_ACCESS_NET + rule_small_size = rule_size - sizeof(uint64_t) - 1; +#else + rule_small_size = rule_size - 1; +#endif + + rule_big_size = SAFE_SYSCONF(_SC_PAGESIZE) + 1; +} + +static struct tst_test test = { + .test = run, + .tcnt = ARRAY_SIZE(tcases), + .setup = setup, + .min_kver = "5.13", + .needs_root = 1, + .needs_kconfigs = (const char *[]) { + "CONFIG_SECURITY_LANDLOCK=y", + NULL + }, + .bufs = (struct tst_buffers []) { + {&ruleset_attr, .size = sizeof(struct landlock_ruleset_attr)}, + {}, + }, + .caps = (struct tst_cap []) { + TST_CAP(TST_CAP_REQ, CAP_SYS_ADMIN), + {} + }, +}; diff --git a/testcases/kernel/syscalls/landlock/landlock_common.h b/testcases/kernel/syscalls/landlock/landlock_common.h new file mode 100644 index 000000000..66f8fd19a --- /dev/null +++ b/testcases/kernel/syscalls/landlock/landlock_common.h @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com> + */ + +#ifndef LANDLOCK_COMMON_H + +#include "tst_test.h" +#include "lapi/prctl.h" +#include "lapi/fcntl.h" +#include "lapi/landlock.h" + +static inline void verify_landlock_is_enabled(void) +{ + int abi; + + abi = tst_syscall(__NR_landlock_create_ruleset, + NULL, 0, LANDLOCK_CREATE_RULESET_VERSION); + + if (abi < 0) { + if (errno == EOPNOTSUPP) { + tst_brk(TCONF, "Landlock is currently disabled. " + "Please enable it either via CONFIG_LSM or " + "'lsm' kernel parameter."); + } + + tst_brk(TBROK | TERRNO, "landlock_create_ruleset error"); + } + + tst_res(TINFO, "Landlock ABI v%d", abi); +} + +static inline void apply_landlock_rule( + struct landlock_path_beneath_attr *path_beneath_attr, + const int ruleset_fd, + const int access, + const char *path) +{ + path_beneath_attr->allowed_access = access; + path_beneath_attr->parent_fd = SAFE_OPEN(path, O_PATH | O_CLOEXEC); + + SAFE_LANDLOCK_ADD_RULE( + ruleset_fd, + LANDLOCK_RULE_PATH_BENEATH, + path_beneath_attr, + 0); + + SAFE_CLOSE(path_beneath_attr->parent_fd); +} + +static inline void enforce_ruleset(const int ruleset_fd) +{ + SAFE_PRCTL(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); + SAFE_LANDLOCK_RESTRICT_SELF(ruleset_fd, 0); +} + +static inline void apply_landlock_layer( + struct landlock_ruleset_attr *ruleset_attr, + struct landlock_path_beneath_attr *path_beneath_attr, + const char *path, + const int access) +{ + int ruleset_fd; + + ruleset_fd = SAFE_LANDLOCK_CREATE_RULESET( + ruleset_attr, sizeof(struct landlock_ruleset_attr), 0); + + apply_landlock_rule(path_beneath_attr, ruleset_fd, access, path); + enforce_ruleset(ruleset_fd); + + SAFE_CLOSE(ruleset_fd); +} + +#endif