On Thu, Apr 11, 2024 at 06:58:34PM +0300, Ivanov Mikhail wrote:
>
> 4/8/2024 4:08 PM, Günther Noack wrote:
> > Hello!
> >
> > I am very happy to see this patch set, this is a very valuable feature, IMHO! :)
> >
> > Regarding the subject of this patch:
> >
> > [RFC PATCH v1 03/10] selftests/landlock: Create 'create' test
> > ^^^^^^
> >
> > This was probably meant to say "socket"?
>
> I wanted each such patch to have the name of the test that this patch
> adds (without specifying the fixture, since this is not necessary
> information, which only complicates the name). I think
>
> [RFC PATCH v1 03/10] selftests/landlock: Add 'create' test
> ~~~
> renaming should be fine.
Maybe something like this?
"selftests/landlock: Add protocol.create to socket tests"
>
> >
> > (In my mind, it is a good call to put the test in a separate file -
> > the existing test files have grown too large already.)
> >
> >
> > On Mon, Apr 08, 2024 at 05:39:20PM +0800, Ivanov Mikhail wrote:
> > > Initiate socket_test.c selftests. Add protocol fixture for tests
> > > with changeable domain/type values. Only most common variants of
> > > protocols (like ipv4-tcp,ipv6-udp, unix) were added.
> > > Add simple socket access right checking test.
> > >
> > > Signed-off-by: Ivanov Mikhail <ivanov.mikhail1@xxxxxxxxxxxxxxxxxxx>
> > > Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx>
> > > ---
> > > .../testing/selftests/landlock/socket_test.c | 197 ++++++++++++++++++
> > > 1 file changed, 197 insertions(+)
> > > create mode 100644 tools/testing/selftests/landlock/socket_test.c
> > >
> > > diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> > > new file mode 100644
> > > index 000000000..525f4f7df
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/landlock/socket_test.c
> > > @@ -0,0 +1,197 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Landlock tests - Socket
> > > + *
> > > + * Copyright © 2024 Huawei Tech. Co., Ltd.
> > > + */
> > > +
> > > +#define _GNU_SOURCE
> > > +
> > > +#include <errno.h>
> > > +#include <linux/landlock.h>
> > > +#include <sched.h>
> > > +#include <string.h>
> > > +#include <sys/prctl.h>
> > > +#include <sys/socket.h>
> > > +
> > > +#include "common.h"
> > > +
> > > +/* clang-format off */
> > > +
> > > +#define ACCESS_LAST LANDLOCK_ACCESS_SOCKET_CREATE
> > > +
> > > +#define ACCESS_ALL ( \
> > > + LANDLOCK_ACCESS_SOCKET_CREATE)
> > > +
> > > +/* clang-format on */
> > > +
> > > +struct protocol_variant {
> > > + int domain;
> > > + int type;
> > > +};
> > > +
> > > +struct service_fixture {
> > > + struct protocol_variant protocol;
> > > +};
> > > +
> > > +static void setup_namespace(struct __test_metadata *const _metadata)
> > > +{
> > > + set_cap(_metadata, CAP_SYS_ADMIN);
> > > + ASSERT_EQ(0, unshare(CLONE_NEWNET));
> > > + clear_cap(_metadata, CAP_SYS_ADMIN);
> > > +}
> > > +
> > > +static int socket_variant(const struct service_fixture *const srv)
> > > +{
> > > + int ret;
> > > +
> > > + ret = socket(srv->protocol.domain, srv->protocol.type | SOCK_CLOEXEC,
> > > + 0);
> > > + if (ret < 0)
> > > + return -errno;
> > > + return ret;
> > > +}
> >
> > This helper is mostly concerned with mapping the error code.
> >
> > In the fs_test.c, we have dealt with such use cases with helpers like
> > test_open_rel() and test_open(). These helpers attempt to open the file, take
> > the same arguments as open(2), but instead of returning the opened fd, they only
> > return 0 or errno. Do you think this would be an option here?
> >
> > Then you could write your tests as
> >
> > ASSERT_EQ(EACCES, test_socket(p->domain, p->type, 0));
> >
> > and the test would (a) more obviously map to socket(2), and (b) keep relevant
> > information like the expected error code at the top level of the test.
> >
>
> I thought that `socket_variant()` would be suitable for future tests
> where sockets can be used after creation (e.g. for sending FDs). But
> until then, it's really better to replace it with what you suggested.
You can move common code/helpers from net_test.c to common.h (with a
dedicated patch) to avoid duplicating code.
>
> > > +
> > > +FIXTURE(protocol)
> > > +{
> > > + struct service_fixture srv0;
> > > +};
> > > +
> > > +FIXTURE_VARIANT(protocol)
> > > +{
> > > + const struct protocol_variant protocol;
> > > +};
> > > +
> > > +FIXTURE_SETUP(protocol)
> > > +{
> > > + disable_caps(_metadata);
> > > + self->srv0.protocol = variant->protocol;
> > > + setup_namespace(_metadata);
> > > +};
> > > +
> > > +FIXTURE_TEARDOWN(protocol)
> > > +{
> > > +}
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, unspec) {
> > > + /* clang-format on */
> > > + .protocol = {
> > > + .domain = AF_UNSPEC,
> > > + .type = SOCK_STREAM,
> > > + },
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, unix_stream) {
> > > + /* clang-format on */
> > > + .protocol = {
> > > + .domain = AF_UNIX,
> > > + .type = SOCK_STREAM,
> > > + },
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, unix_dgram) {
> > > + /* clang-format on */
> > > + .protocol = {
> > > + .domain = AF_UNIX,
> > > + .type = SOCK_DGRAM,
> > > + },
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, ipv4_tcp) {
> > > + /* clang-format on */
> > > + .protocol = {
> > > + .domain = AF_INET,
> > > + .type = SOCK_STREAM,
> > > + },
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, ipv4_udp) {
> > > + /* clang-format on */
> > > + .protocol = {
> > > + .domain = AF_INET,
> > > + .type = SOCK_DGRAM,
> > > + },
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, ipv6_tcp) {
> > > + /* clang-format on */
> > > + .protocol = {
> > > + .domain = AF_INET6,
> > > + .type = SOCK_STREAM,
> > > + },
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, ipv6_udp) {
> > > + /* clang-format on */
> > > + .protocol = {
> > > + .domain = AF_INET6,
> > > + .type = SOCK_DGRAM,
> > > + },
> > > +};
> > > +
> > > +static void test_socket_create(struct __test_metadata *const _metadata,
> > > + const struct service_fixture *const srv,
> > > + const bool deny_create)
> > > +{
> > > + int fd;
> > > +
> > > + fd = socket_variant(srv);
> > > + if (srv->protocol.domain == AF_UNSPEC) {
> > > + EXPECT_EQ(fd, -EAFNOSUPPORT);
> > > + } else if (deny_create) {
> > > + EXPECT_EQ(fd, -EACCES);
The deny_create argument/check should not be in this helper but in the
caller.
> > > + } else {
> > > + EXPECT_LE(0, fd)
This should be an ASSERT because the following code using fd would make
no sense if executed.
> > > + {
> > > + TH_LOG("Failed to create socket: %s", strerror(errno));
> > > + }
> > > + EXPECT_EQ(0, close(fd));
> > > + }
> > > +}
> >
> > This is slightly too much logic in a test helper, for my taste,
> > and the meaning of the true/false argument in the main test below
> > is not very obvious.
> >
> > Extending the idea from above, if test_socket() was simpler, would it
> > be possible to turn these fixtures into something shorter like this:
> >
> > ASSERT_EQ(EAFNOSUPPORT, test_socket(AF_UNSPEC, SOCK_STREAM, 0));
> > ASSERT_EQ(EACCES, test_socket(AF_UNIX, SOCK_STREAM, 0));
> > ASSERT_EQ(EACCES, test_socket(AF_UNIX, SOCK_DGRAM, 0));
> > ASSERT_EQ(EACCES, test_socket(AF_INET, SOCK_STREAM, 0));
> > // etc.
I'd prefer that too.
> >
> > Would that make the tests easier to write, to list out the table of
> > expected values aspect like that, rather than in a fixture?
> >
> >
>
> Initially, I conceived this function as an entity that allows to
> separate the logic associated with the tested methods or usecases from
> the logic of configuring the state of the Landlock ruleset in the
> sandbox.
>
> But at the moment, `test_socket_create()` is obviously a wrapper over
> socket(2). So for now it's worth removing unnecessary logic.
>
> But i don't think it's worth removing the fixtures here:
>
> * in my opinion, the design of the fixtures is quite convenient.
> It allows you to separate the definition of the object under test
> from the test case. E.g. test protocol.create checks the ability of
> Landlock to restrict the creation of a socket of a certain type,
> rather than the ability to restrict creation of UNIX, TCP, UDP...
> sockets
I'm not sure to understand, but we need to have positive and negative
tests, potentially in separate TEST_F().
>
> * with adding more tests, it may be necessary to check all protocols
> in each of them
>
> AF_UNSPEC should be removed from fixture variant to separate test,
> though.
Or you can add a new field to mark it as such.
A test should check that AF_UNSPEC cannot be restricted though.
>
> > > +
> > > +TEST_F(protocol, create)
> > > +{
> > > + const struct landlock_ruleset_attr ruleset_attr = {
> > > + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> > > + };
> > > + const struct landlock_socket_attr create_socket_attr = {
> > > + .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> > > + .domain = self->srv0.protocol.domain,
> > > + .type = self->srv0.protocol.type,
> > > + };
> > > +
> > > + int ruleset_fd;
> > > +
> > > + /* Allowed create */
> > > + ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> > > + sizeof(ruleset_attr), 0);
> > > + ASSERT_LE(0, ruleset_fd);
> > > +
> > > + ASSERT_EQ(0,
> > > + landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> > > + &create_socket_attr, 0));
> >
> > The indentation looks wrong? We run clang-format on Landlock files.
> >
> > clang-format -i security/landlock/*.[ch] \
> > include/uapi/linux/landlock.h \
> > tools/testing/selftests/landlock/*.[ch]
> >
>
> Thanks! I'll fix indentation in the patch.
Please fix formatting in all patches.
You should have enough for a second patch series. :)
