Message ID | 20240701-landlock-v1-5-58e9af649a72@suse.com |
---|---|
State | Superseded |
Headers | show |
Series | landlock testing suite | expand |
On Mon, Jul 1, 2024 at 11:43 PM Andrea Cervesato <andrea.cervesato@suse.de> wrote: > From: Andrea Cervesato <andrea.cervesato@suse.com> > Reviewed-by: Li Wang <liwang@redhat.com> Mostly code is pretty good, but just a tiny issue comment inline below. > 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) > > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> > --- > runtest/syscalls | 2 + > testcases/kernel/syscalls/landlock/.gitignore | 1 + > testcases/kernel/syscalls/landlock/Makefile | 10 +++ > testcases/kernel/syscalls/landlock/landlock01.c | 87 > ++++++++++++++++++++++ > .../kernel/syscalls/landlock/landlock_common.h | 74 ++++++++++++++++++ > 5 files changed, 174 insertions(+) > > diff --git a/runtest/syscalls b/runtest/syscalls > index 44a577db3..4c566d95f 100644 > --- a/runtest/syscalls > +++ b/runtest/syscalls > @@ -684,6 +684,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..4b3e3fd8f > --- /dev/null > +++ b/testcases/kernel/syscalls/landlock/Makefile > @@ -0,0 +1,10 @@ > +# 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 > + > +LDLIBS += -lc > + > +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..9f8c6489c > --- /dev/null > +++ b/testcases/kernel/syscalls/landlock/landlock01.c > @@ -0,0 +1,87 @@ > +// 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); > + > + rule_small_size = rule_size - 1; > Here minus 1 is not enough, we have to decrease more than 8 otherwise it just returns ENOMSG. landlock01.c:49: TFAIL: Size is too small expected EINVAL: ENOMSG (42) rule_small_size = rule_size - 9; or, we use a small enough type: rule_small_size = sizeof(int); my test kernel is 6.9.6-200.fc40.x86_64. I haven't figured out that's why, but maybe inside the landlock_create_ruleset() syscall compare the size unprecisely. Something like invoke the copy_min_struct_from_user. 199 err = copy_min_struct_from_user(&ruleset_attr, sizeof(ruleset_attr), 200 offsetofend(typeof(ruleset_attr), 201 handled_access_fs), 202 attr, size); ... 32 #define offsetofend(TYPE, MEMBER) \ 33 (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER)) This needs more investigation... > + 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 > > -- > 2.43.0 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > >
On Tue, Jul 2, 2024 at 4:34 PM Li Wang <liwang@redhat.com> wrote: > > > On Mon, Jul 1, 2024 at 11:43 PM Andrea Cervesato <andrea.cervesato@suse.de> > wrote: > >> From: Andrea Cervesato <andrea.cervesato@suse.com> >> > > Reviewed-by: Li Wang <liwang@redhat.com> > > Mostly code is pretty good, but just a tiny issue comment inline below. > > >> 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) >> >> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> >> --- >> runtest/syscalls | 2 + >> testcases/kernel/syscalls/landlock/.gitignore | 1 + >> testcases/kernel/syscalls/landlock/Makefile | 10 +++ >> testcases/kernel/syscalls/landlock/landlock01.c | 87 >> ++++++++++++++++++++++ >> .../kernel/syscalls/landlock/landlock_common.h | 74 >> ++++++++++++++++++ >> 5 files changed, 174 insertions(+) >> >> diff --git a/runtest/syscalls b/runtest/syscalls >> index 44a577db3..4c566d95f 100644 >> --- a/runtest/syscalls >> +++ b/runtest/syscalls >> @@ -684,6 +684,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..4b3e3fd8f >> --- /dev/null >> +++ b/testcases/kernel/syscalls/landlock/Makefile >> @@ -0,0 +1,10 @@ >> +# 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 >> + >> +LDLIBS += -lc >> + >> +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..9f8c6489c >> --- /dev/null >> +++ b/testcases/kernel/syscalls/landlock/landlock01.c >> @@ -0,0 +1,87 @@ >> +// 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); >> + >> + rule_small_size = rule_size - 1; >> > > Here minus 1 is not enough, we have to decrease more than 8 > otherwise it just returns ENOMSG. > > landlock01.c:49: TFAIL: Size is too small expected EINVAL: ENOMSG (42) > > rule_small_size = rule_size - 9; > > or, we use a small enough type: > > rule_small_size = sizeof(int); > > my test kernel is 6.9.6-200.fc40.x86_64. > > I haven't figured out that's why, but maybe inside the > landlock_create_ruleset() > syscall compare the size unprecisely. Something like invoke the > copy_min_struct_from_user. > > 199 err = copy_min_struct_from_user(&ruleset_attr, > sizeof(ruleset_attr), > 200 > offsetofend(typeof(ruleset_attr), > 201 handled_access_fs), > 202 attr, size); > > ... > > 32 #define offsetofend(TYPE, MEMBER) \ > 33 (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER)) > > This needs more investigation... > I guess I found the reason, kernel commit fff69fb03dde1df introducing a new field 'handled_access_net' in the structure, but in the landlock_create_ruleset(), it still uses the first field 'handled_access_fs' to count min size, so that why decrease 1 is useless in your test. I will send out a RFC patch to LKML for that. Thanks! --- a/security/landlock/syscalls.c +++ b/security/landlock/syscalls.c @@ -198,7 +198,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset, /* Copies raw user space buffer. */ err = copy_min_struct_from_user(&ruleset_attr, sizeof(ruleset_attr), offsetofend(typeof(ruleset_attr), - handled_access_fs), + handled_access_net), attr, size); if (err) return err; >
Hi! On 7/2/24 11:09, Li Wang wrote: > > > On Tue, Jul 2, 2024 at 4:34 PM Li Wang <liwang@redhat.com> wrote: > > > > On Mon, Jul 1, 2024 at 11:43 PM Andrea Cervesato > <andrea.cervesato@suse.de> wrote: > > From: Andrea Cervesato <andrea.cervesato@suse.com> > > > Reviewed-by: Li Wang <liwang@redhat.com> > > Mostly code is pretty good, but just a tiny issue comment inline > below. > > > 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) > > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> > --- > runtest/syscalls | 2 + > testcases/kernel/syscalls/landlock/.gitignore | 1 + > testcases/kernel/syscalls/landlock/Makefile | 10 +++ > testcases/kernel/syscalls/landlock/landlock01.c | 87 > ++++++++++++++++++++++ > .../kernel/syscalls/landlock/landlock_common.h | 74 > ++++++++++++++++++ > 5 files changed, 174 insertions(+) > > diff --git a/runtest/syscalls b/runtest/syscalls > index 44a577db3..4c566d95f 100644 > --- a/runtest/syscalls > +++ b/runtest/syscalls > @@ -684,6 +684,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..4b3e3fd8f > --- /dev/null > +++ b/testcases/kernel/syscalls/landlock/Makefile > @@ -0,0 +1,10 @@ > +# 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 > <http://testcases.mk> > + > +LDLIBS += -lc > + > +include $(top_srcdir)/include/mk/generic_leaf_target.mk > <http://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..9f8c6489c > --- /dev/null > +++ b/testcases/kernel/syscalls/landlock/landlock01.c > @@ -0,0 +1,87 @@ > +// 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); > + > + rule_small_size = rule_size - 1; > > > Here minus 1 is not enough, we have to decrease more than 8 > otherwise it just returns ENOMSG. > > landlock01.c:49: TFAIL: Size is too small expected EINVAL: > ENOMSG (42) > > rule_small_size = rule_size - 9; > > or, we use a small enough type: > > rule_small_size = sizeof(int); > > my test kernel is 6.9.6-200.fc40.x86_64. > > I haven't figured out that's why, but maybe inside the > landlock_create_ruleset() > syscall compare the size unprecisely. Something like invoke the > copy_min_struct_from_user. > > 199 err = copy_min_struct_from_user(&ruleset_attr, > sizeof(ruleset_attr), > 200 offsetofend(typeof(ruleset_attr), > 201 handled_access_fs), > 202 attr, size); > > ... > > 32 #define offsetofend(TYPE, MEMBER) \ > 33 (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER)) > > This needs more investigation... > > > I guess I found the reason, kernel commit fff69fb03dde1df introducing > a new > field 'handled_access_net' in the structure, but in the > landlock_create_ruleset(), > it still uses the first field 'handled_access_fs' to count min size, > so that why > decrease 1 is useless in your test. > > I will send out a RFC patch to LKML for that. Thanks! > > --- a/security/landlock/syscalls.c > +++ b/security/landlock/syscalls.c > @@ -198,7 +198,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset, > /* Copies raw user space buffer. */ > err = copy_min_struct_from_user(&ruleset_attr, > sizeof(ruleset_attr), > offsetofend(typeof(ruleset_attr), > - handled_access_fs), > + handled_access_net), > attr, size); > if (err) > return err; > > > > -- > Regards, > Li Wang First of all, thanks for the review. Super for the bug in the landlock code. Regards, Andrea Cervesato
Andrea Cervesato <andrea.cervesato@suse.com> wrote: > +static void setup(void) >>> +{ >>> + verify_landlock_is_enabled(); >>> + >>> + rule_size = sizeof(struct landlock_ruleset_attr); >>> + >>> + rule_small_size = rule_size - 1; >>> >> >> Here minus 1 is not enough, we have to decrease more than 8 >> otherwise it just returns ENOMSG. >> >> landlock01.c:49: TFAIL: Size is too small expected EINVAL: ENOMSG (42) >> >> rule_small_size = rule_size - 9; >> >> or, we use a small enough type: >> >> rule_small_size = sizeof(int); >> >> my test kernel is 6.9.6-200.fc40.x86_64. >> >> I haven't figured out that's why, but maybe inside the >> landlock_create_ruleset() >> syscall compare the size unprecisely. Something like invoke the >> copy_min_struct_from_user. >> >> 199 err = copy_min_struct_from_user(&ruleset_attr, >> sizeof(ruleset_attr), >> 200 >> offsetofend(typeof(ruleset_attr), >> 201 handled_access_fs), >> 202 attr, size); >> >> ... >> >> 32 #define offsetofend(TYPE, MEMBER) \ >> 33 (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER)) >> >> This needs more investigation... >> > > I guess I found the reason, kernel commit fff69fb03dde1df introducing a new > field 'handled_access_net' in the structure, but in the > landlock_create_ruleset(), > it still uses the first field 'handled_access_fs' to count min size, so > that why > decrease 1 is useless in your test. > > I will send out a RFC patch to LKML for that. Thanks! > > --- a/security/landlock/syscalls.c > +++ b/security/landlock/syscalls.c > @@ -198,7 +198,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset, > /* Copies raw user space buffer. */ > err = copy_min_struct_from_user(&ruleset_attr, > sizeof(ruleset_attr), > offsetofend(typeof(ruleset_attr), > - handled_access_fs), > + handled_access_net), > attr, size); > if (err) > return err; > >> > > -- > Regards, > Li Wang > > First of all, thanks for the review. Super for the bug in the landlock > code. > No problem, but that's maybe on purpose set only one field for the structure's minimal size, if so, we have to adjust your testcase to decrese 9. Let's see what the author thinks about my proposal patch[1]. [1] https://lists.linux.it/pipermail/ltp/2024-July/039073.html
diff --git a/runtest/syscalls b/runtest/syscalls index 44a577db3..4c566d95f 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -684,6 +684,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..4b3e3fd8f --- /dev/null +++ b/testcases/kernel/syscalls/landlock/Makefile @@ -0,0 +1,10 @@ +# 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 + +LDLIBS += -lc + +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..9f8c6489c --- /dev/null +++ b/testcases/kernel/syscalls/landlock/landlock01.c @@ -0,0 +1,87 @@ +// 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); + + rule_small_size = rule_size - 1; + 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