[05/10] Add landlock01 test

From: Andrea Cervesato <andrea.cervesato@suse.com>

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

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