From: "Günther Noack" <gnoack@google•com>
To: Mikhail Ivanov <ivanov.mikhail1@huawei-partners•com>
Cc: mic@digikod•net, willemdebruijn.kernel@gmail•com,
gnoack3000@gmail•com, linux-security-module@vger•kernel.org,
netdev@vger•kernel.org, netfilter-devel@vger•kernel.org,
yusongping@huawei•com, artem.kuzin@huawei•com,
konstantin.meskhidze@huawei•com
Subject: Re: [RFC PATCH v3 06/19] selftests/landlock: Test adding a rule for unhandled access
Date: Fri, 13 Sep 2024 17:04:10 +0200 [thread overview]
Message-ID: <ZuRUagjolNjXsS3r@google.com> (raw)
In-Reply-To: <fd6ef478-4d0b-03f2-78f6-8bfd0fc3a846@huawei-partners.com>
On Wed, Sep 11, 2024 at 11:19:48AM +0300, Mikhail Ivanov wrote:
> On 9/10/2024 12:22 PM, Günther Noack wrote:
> > Hi!
> >
> > On Wed, Sep 04, 2024 at 06:48:11PM +0800, Mikhail Ivanov wrote:
> > > Add test that validates behaviour of Landlock after rule with
> > > unhandled access is added.
> > >
> > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners•com>
> > > ---
> > > Changes since v2:
> > > * Replaces EXPECT_EQ with ASSERT_EQ for close().
> > > * Refactors commit title and message.
> > >
> > > Changes since v1:
> > > * Refactors commit message.
> > > ---
> > > .../testing/selftests/landlock/socket_test.c | 33 +++++++++++++++++++
> > > 1 file changed, 33 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> > > index 811bdaa95a7a..d2fedfca7193 100644
> > > --- a/tools/testing/selftests/landlock/socket_test.c
> > > +++ b/tools/testing/selftests/landlock/socket_test.c
> > > @@ -351,4 +351,37 @@ TEST_F(protocol, rule_with_unknown_access)
> > > ASSERT_EQ(0, close(ruleset_fd));
> > > }
> > > +TEST_F(protocol, rule_with_unhandled_access)
> > > +{
> > > + struct landlock_ruleset_attr ruleset_attr = {
> > > + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> > > + };
> > > + struct landlock_socket_attr protocol = {
> > > + .family = self->prot.family,
> > > + .type = self->prot.type,
> > > + };
> > > + int ruleset_fd;
> > > + __u64 access;
> > > +
> > > + ruleset_fd =
> > > + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> > > + ASSERT_LE(0, ruleset_fd);
> > > +
> > > + for (access = 1; access > 0; access <<= 1) {
> > > + int err;
> > > +
> > > + protocol.allowed_access = access;
> > > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> > > + &protocol, 0);
> > > + if (access == ruleset_attr.handled_access_socket) {
> > > + EXPECT_EQ(0, err);
> > > + } else {
> > > + EXPECT_EQ(-1, err);
> > > + EXPECT_EQ(EINVAL, errno);
> > > + }
> > > + }
> > > +
> > > + ASSERT_EQ(0, close(ruleset_fd));
> > > +}
> > > +
> >
> > I should probably have noticed this on the first review round; you are not
> > actually exercising any scenario here where a rule with unhandled access is
> > added.
> >
> > To clarify, the notion of an access right being "unhandled" means that the
> > access right was not listed at ruleset creation time in the ruleset_attr's
> > .handled_access_* field where it would have belonged. If that is the case,
> > adding a ruleset with that access right is going to be denied.
> >
> > As an example:
> > If the ruleset only handles LANDLOCK_ACCESS_FS_WRITE_FILE and nothing else,
> > then, if the test tries to insert a rule for LANDLOCK_ACCESS_SOCKET_CREATE,
> > that call is supposed to fail -- because the "socket creation" access right is
> > not handled.
>
> This test was added to exercise adding a rule with future possible
> "unhandled" access rights of "socket" type, but since this patch
> implements only one, this test is really meaningless. Thank you for
> this note!
>
> >
> > IMHO the test would become more reasonable if it was more clearly "handling"
> > something entirely unrelated at ruleset creation time, e.g. one of the file
> > system access rights. (And we could do the same for the "net" and "fs" tests as
> > well.)
> >
> > Your test is a copy of the same test for the "net" rights, which in turn is a
> > copy of teh same test for the "fs" rights. When the "fs" test was written, the
> > "fs" access rights were the only ones that could be used at all to create a
> > ruleset, but this is not true any more.
>
> Good idea! Can I implement such test in the current patchset?
Yes, I think it would be a good idea.
I would, in fact, recommend to turn the rule_with_unhandled_access test into that test.
The test traces its roots clearly to
TEST_F(mini, rule_with_unhandled_access) from net_test.c
and to
TEST_F_FORK(layout1, rule_with_unhandled_access) from fs_test.c
and I think all three variants would better be advised to create a ruleset with
struct landlock_ruleset_attr ruleset_attr = {
.handled_access_something_entirely_different = LANDLOCK_ACCESS_WHATEVER,
}
and then check their corresponding fs, net and socket access rights using a
landlock_add_rule() call for the access rights that belong to the respective
module, so that it exercises the scenario where userspace attempts to use the
access right in a rule, but the surrounding ruleset did not restrict the same
access right (it was "unhandled").
In spirit, it would be nicest if we could create a ruleset where nothing at all
is handled, but I believe in that case, the landlock_create_ruleset() call would
already fail.
—Günther
P.S.: I am starting to grow a bit uncomfortable with the amount of duplicated
test code that we start having across the different types of access rights. If
you see a way to keep this more in check, while still keeping the tests
expressive and not over-frameworking them, let's try to move in that direction
if we can. :)
next prev parent reply other threads:[~2024-09-13 15:04 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-04 10:48 [RFC PATCH v3 00/19] Support socket access-control Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 01/19] landlock: " Mikhail Ivanov
2024-09-06 13:09 ` Günther Noack
2024-09-09 7:23 ` Mikhail Ivanov
2024-11-11 16:29 ` Mikhail Ivanov
2024-11-22 17:45 ` Günther Noack
2024-11-25 11:04 ` Mikhail Ivanov
2024-11-27 18:43 ` Mickaël Salaün
2024-11-28 12:01 ` Mikhail Ivanov
2024-11-28 20:52 ` Mickaël Salaün
2024-12-02 11:32 ` Mikhail Ivanov
2024-12-24 16:55 ` Mikhail Ivanov
2025-01-10 11:12 ` Günther Noack
2025-01-10 13:02 ` Mikhail Ivanov
2025-01-10 16:27 ` Günther Noack
2025-01-10 16:55 ` Mikhail Ivanov
2025-01-14 18:31 ` Mickaël Salaün
2025-01-24 12:28 ` Mikhail Ivanov
2025-01-24 14:02 ` Mickaël Salaün
2024-09-04 10:48 ` [RFC PATCH v3 02/19] landlock: Add hook on socket creation Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 03/19] selftests/landlock: Test basic socket restriction Mikhail Ivanov
2024-09-10 9:53 ` Günther Noack
2024-09-04 10:48 ` [RFC PATCH v3 04/19] selftests/landlock: Test adding a rule with each supported access Mikhail Ivanov
2024-09-10 9:53 ` Günther Noack
2024-09-04 10:48 ` [RFC PATCH v3 05/19] selftests/landlock: Test adding a rule for each unknown access Mikhail Ivanov
2024-09-10 9:53 ` Günther Noack
2024-09-04 10:48 ` [RFC PATCH v3 06/19] selftests/landlock: Test adding a rule for unhandled access Mikhail Ivanov
2024-09-10 9:22 ` Günther Noack
2024-09-11 8:19 ` Mikhail Ivanov
2024-09-13 15:04 ` Günther Noack [this message]
2024-09-13 16:15 ` Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 07/19] selftests/landlock: Test adding a rule for empty access Mikhail Ivanov
2024-09-18 12:42 ` Günther Noack
2024-09-18 13:03 ` Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 08/19] selftests/landlock: Test overlapped restriction Mikhail Ivanov
2024-09-18 12:42 ` Günther Noack
2024-09-04 10:48 ` [RFC PATCH v3 09/19] selftests/landlock: Test creating a ruleset with unknown access Mikhail Ivanov
2024-09-18 12:44 ` Günther Noack
2024-09-04 10:48 ` [RFC PATCH v3 10/19] selftests/landlock: Test adding a rule with family and type outside the range Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 11/19] selftests/landlock: Test unsupported protocol restriction Mikhail Ivanov
2024-09-18 12:54 ` Günther Noack
2024-09-18 13:36 ` Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 12/19] selftests/landlock: Test that kernel space sockets are not restricted Mikhail Ivanov
2024-09-04 12:45 ` Mikhail Ivanov
2024-09-18 13:00 ` Günther Noack
2024-09-19 10:53 ` Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 13/19] selftests/landlock: Test packet protocol alias Mikhail Ivanov
2024-09-18 13:33 ` Günther Noack
2024-09-18 14:01 ` Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 14/19] selftests/landlock: Test socketpair(2) restriction Mikhail Ivanov
2024-09-18 13:47 ` Günther Noack
2024-09-23 12:57 ` Mikhail Ivanov
2024-09-25 12:17 ` Mikhail Ivanov
2024-09-27 9:48 ` Günther Noack
2024-09-28 20:06 ` Günther Noack
2024-09-29 17:31 ` Mickaël Salaün
2024-10-03 17:27 ` Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 15/19] selftests/landlock: Test SCTP peeloff restriction Mikhail Ivanov
2024-09-27 14:35 ` Günther Noack
2024-10-03 12:15 ` Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 16/19] selftests/landlock: Test that accept(2) is not restricted Mikhail Ivanov
2024-09-27 14:53 ` Günther Noack
2024-10-03 12:41 ` Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 17/19] samples/landlock: Replace atoi() with strtoull() in populate_ruleset_net() Mikhail Ivanov
2024-09-27 15:12 ` Günther Noack
2024-10-03 12:59 ` Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 18/19] samples/landlock: Support socket protocol restrictions Mikhail Ivanov
2024-10-01 7:56 ` Günther Noack
2024-10-03 13:15 ` Mikhail Ivanov
2024-09-04 10:48 ` [RFC PATCH v3 19/19] landlock: Document socket rule type support Mikhail Ivanov
2024-10-01 7:09 ` Günther Noack
2024-10-03 14:00 ` Mikhail Ivanov
2024-10-03 16:21 ` Günther Noack
2025-04-22 17:19 ` [RFC PATCH v3 00/19] Support socket access-control Mickaël Salaün
2025-04-25 13:58 ` Günther Noack
2025-04-29 11:59 ` Mikhail Ivanov
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=ZuRUagjolNjXsS3r@google.com \
--to=gnoack@google$(echo .)com \
--cc=artem.kuzin@huawei$(echo .)com \
--cc=gnoack3000@gmail$(echo .)com \
--cc=ivanov.mikhail1@huawei-partners$(echo .)com \
--cc=konstantin.meskhidze@huawei$(echo .)com \
--cc=linux-security-module@vger$(echo .)kernel.org \
--cc=mic@digikod$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=netfilter-devel@vger$(echo .)kernel.org \
--cc=willemdebruijn.kernel@gmail$(echo .)com \
--cc=yusongping@huawei$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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