From: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>
To: Dorine Tipo <dorine.a.tipo@gmail.com>
Cc: mic@digikod.net, skhan@linuxfoundation.org,
outreachy@lists.linux.dev, Dorine Tipo <dorine.a.tipo@gmail.com>
Subject: Re: [PATCH v2] Add Landlock test for io_uring IORING_OP_OPENAT operation
Date: Thu, 28 Mar 2024 17:04:33 +0100 [thread overview]
Message-ID: <5043999.MHq7AAxBmi@fdefranc-mobl3> (raw)
In-Reply-To: <20240327132001.30576-1-dorine.a.tipo@gmail.com>
On Wednesday, 27 March 2024 14:20:01 CET Dorine Tipo wrote:
> This patch expands Landlock test coverage to include io_uring operations.
> It introduces a test for IORING_OP_OPENAT with Landlock rules, verifying
> allowed and disallowed access. This mitigates potential security
> vulnerabilities by ensuring Landlock controls access through io_uring.
>
> It also updates the Makefile to include -luring in the LDLIBS.
> This ensures the test code has access to the necessary liburing
> library for io_uring operations.
>
> Signed-off-by: Dorine Tipo <dorine.a.tipo@gmail.com>
> ---
Hi Dorine,
I'm adding inline a few comments and questions for you. However, since my
experience with io_uring (inexistent with Landlock) is limited, I won't
comment on the use of its API.
> Changes since V1:
> V2: - Consolidated two dependent patches in the V1 series into one patch
> as suggested by <fabio.maria.de.francesco@linux.intel.com>
NIT: No need for an entire email address. Next time"Fabio" will suffice :)
> - Updated the subject line to be more descriptive.
>
> tools/testing/selftests/landlock/Makefile | 4 +-
> tools/testing/selftests/landlock/fs_test.c | 132 +++++++++++++++++++++
> 2 files changed, 134 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/landlock/Makefile
> b/tools/testing/selftests/landlock/Makefile index
> 348e2dbdb4e0..ab47d1dadb62 100644
> --- a/tools/testing/selftests/landlock/Makefile
> +++ b/tools/testing/selftests/landlock/Makefile
> @@ -13,11 +13,11 @@ TEST_GEN_PROGS := $(src_test:.c=)
> TEST_GEN_PROGS_EXTENDED := true
>
> # Short targets:
> -$(TEST_GEN_PROGS): LDLIBS += -lcap
> +$(TEST_GEN_PROGS): LDLIBS += -lcap -luring
> $(TEST_GEN_PROGS_EXTENDED): LDFLAGS += -static
>
> include ../lib.mk
>
> # Targets with $(OUTPUT)/ prefix:
> -$(TEST_GEN_PROGS): LDLIBS += -lcap
> +$(TEST_GEN_PROGS): LDLIBS += -lcap -luring
> $(TEST_GEN_PROGS_EXTENDED): LDFLAGS += -static
> diff --git a/tools/testing/selftests/landlock/fs_test.c
> b/tools/testing/selftests/landlock/fs_test.c index
> 9a6036fbf289..9c8247995d42 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -21,7 +21,10 @@
> #include <sys/stat.h>
> #include <sys/sysmacros.h>
> #include <sys/vfs.h>
> +#include <sys/types.h>
> #include <unistd.h>
> +#include <liburing.h>
> +#include <linux/io_uring.h>
>
> #include "common.h"
>
> @@ -4874,4 +4877,133 @@ TEST_F_FORK(layout3_fs, release_inodes)
> ASSERT_EQ(EACCES, test_open(TMP_DIR, O_RDONLY));
> }
>
> +/* Test io_uring openat access control with landlock rules */
> +static int test_ioring_op_openat(struct __test_metadata *const
> _metadata, const __u64 access, const char **paths, const int paths_size)
"access", "paths", and "paths_size" are not used anywhere. Why are there
three unused arguments in the test_ioring_op_openat() signature?
> +{
> + struct io_uring ring;
> + struct io_uring_sqe *sqe;
> +
> + const char *allowed_paths[] = {
> + file1_s1d1, file2_s1d1,
> + file1_s1d2, file2_s1d2,
> + file1_s1d3, file2_s1d3,
> + file1_s2d1, file1_s2d2,
> + file1_s2d3, file2_s2d3,
> + file1_s3d1,
> + };
> +
> + const char *disallowed_paths[] = {
> + /* dir_s3d2 is a mount point. */
> + dir_s3d2,
> + dir_s3d3,
> + };
> +
> + /* Test Allowed Access */
> + const struct rule allowed_rule[] = {
> + {
> + .path = allowed_paths[0],
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE,
> + },
I suspect that you are forgetting the other elements as well as the final
NULL element. Don't we need to fill allowed_rule[] with the other pairs of
allowed paths and their respective access?
> + };
> + int allowed_ruleset_fd = create_ruleset(_metadata,
> allowed_rule[0].access, allowed_rule);
Please check how create_ruleset() is implemented. And pay special attention
at this snippet:
for (i = 0; rules[i].path; i++) {
add_path_beneath(_metadata, ruleset_fd, rules[i].access,
rules[i].path);
It loops on the elements of rules[], working on the fields of each element
struct (i.e., "path" and "access"), but "allowed_rule" is an array with a
single element.
Furthermore, I think that you are forgetting a NULL final struct that marks
the last element of allowed_rule[]. Am I overlooking something?
> + ASSERT_LE(0, allowed_ruleset_fd);
> +
> + int ret = io_uring_queue_init(32, &ring, 0);
> +
> + ASSERT_EQ(0, ret);
> +
> + /* Test each allowed path */
> + for (int i = 0; i < ARRAY_SIZE(allowed_paths); ++i) {
> + sqe = io_uring_get_sqe(&ring);
> + io_uring_prep_openat(sqe, AT_FDCWD, allowed_paths[i],
O_RDONLY,
> + allowed_ruleset_fd);
> + /* Verify successful SQE preparation */
> + ASSERT_EQ(0, ret);
> +
> + if (ret != 0)
> + return ret;
> +
> + ret = io_uring_submit(&ring);
> + /* Verify 1 submission completed */
> + ASSERT_EQ(1, ret);
> + }
> +
> + /* Test Disallowed Access */
> + const struct rule disallowed_rule[] = {
> + {
> + .path = disallowed_paths[0],
> + .access = 0,
> + }
Again, what happened to the other disallowed path and to the final NULL
element of disallowed_rule[]?
> +
> + };
> + int disallowed_ruleset_fd = create_ruleset(_metadata,
> disallowed_rule[0].access, disallowed_rule); +
> + ASSERT_LE(0, disallowed_ruleset_fd);
> +
> + /* Test each disallowed path */
> + for (int i = 0; i < ARRAY_SIZE(disallowed_paths); ++i) {
> + sqe = io_uring_get_sqe(&ring);
> + io_uring_prep_openat(sqe, AT_FDCWD, disallowed_paths[i],
O_RDONLY,
> disallowed_ruleset_fd); + /* Verify successful SQE
preparation */
> + ASSERT_EQ(1, ret);
> +
> + if (ret != 0)
> + return ret;
> +
> + ret = io_uring_submit(&ring);
> + /* Verify 1 submission completed */
> + ASSERT_EQ(0, ret);
> + }
> +
> + /* Cleanup: close ruleset fds, etc. */
> + close(allowed_ruleset_fd);
> + close(disallowed_ruleset_fd);
> +
> + return 0;
> +}
> +
> +/* clang-format off */
> +FIXTURE(openat_test) {
> + struct __test_metadata *metadata;
> + const char *allowed_paths[11];
> + const char *disallowed_paths[2];
> +};
> +
> +/* clang-format on */
> +
> +FIXTURE_SETUP(openat_test)
> +{
> + /* initialize metadata, allowed_paths, and disallowed_paths */
> + self->metadata = _metadata;
> + const char *temp_allowed_paths[] = {
> + file1_s1d1, file2_s1d1, file1_s1d2, file2_s1d2,
> + file1_s1d3, file2_s1d3, file1_s2d1, file1_s2d2,
> + file1_s2d3, file2_s2d3, file1_s3d1};
> +
Can you please say why you are defining both temp_allowed_paths[] and
allowed_paths[]?
> + memcpy(self->allowed_paths, temp_allowed_paths,
> sizeof(temp_allowed_paths)); +
> + const char *temp_disallowed_paths[] = {dir_s3d2, dir_s3d3};
I have the same question for temp_disallowed_paths[] and
disallowed_paths[].
I see that they are initialised with the same set of information. Why don't
you define them only once and pass references where needed?
Thanks,
Fabio
> +
> + memcpy(self->disallowed_paths, temp_disallowed_paths,
> sizeof(temp_disallowed_paths)); +}
> +
> +FIXTURE_TEARDOWN(openat_test)
> +{
> + /* Clean up test environment */
> +}
> +
> +TEST_F_FORK(openat_test, test_ioring_op_openat_allowed)
> +{
> + test_ioring_op_openat(self->metadata, LANDLOCK_ACCESS_FS_READ_FILE
|
> + LANDLOCK_ACCESS_FS_WRITE_FILE, self-
>allowed_paths,
> + ARRAY_SIZE(self->allowed_paths));
> +}
> +
> +TEST_F_FORK(openat_test, test_ioring_op_openat_disallowed)
> +{
> + test_ioring_op_openat(self->metadata, 0, self->disallowed_paths,
> + ARRAY_SIZE(self->disallowed_paths));
> +}
> +
> TEST_HARNESS_MAIN
> --
> 2.25.1
prev parent reply other threads:[~2024-03-28 16:04 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-27 13:20 [PATCH v2] Add Landlock test for io_uring IORING_OP_OPENAT operation Dorine Tipo
2024-03-27 16:12 ` Shuah Khan
2024-03-28 14:43 ` Mickaël Salaün
2024-03-28 16:04 ` Fabio M. De Francesco [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5043999.MHq7AAxBmi@fdefranc-mobl3 \
--to=fabio.maria.de.francesco@linux.intel.com \
--cc=dorine.a.tipo@gmail.com \
--cc=mic@digikod.net \
--cc=outreachy@lists.linux.dev \
--cc=skhan@linuxfoundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).