| Message ID | 20240408093927.1759381-4-ivanov.mikhail1@huawei-partners.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Socket type control for Landlock | expand |
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"?
(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@huawei-partners.com>
> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> ---
> .../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.
> +
> +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);
> + } else {
> + EXPECT_LE(0, fd)
> + {
> + 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.
Would that make the tests easier to write, to list out the table of
expected values aspect like that, rather than in a fixture?
> +
> +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]
—Günther
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. > > (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@huawei-partners.com> >> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> >> --- >> .../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. >> + >> +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); >> + } else { >> + EXPECT_LE(0, fd) >> + { >> + 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. > > 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 * 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. >> + >> +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. > —Günther
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@huawei-partners.com> > > > Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > > > --- > > > .../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. :)
5/8/2024 1:38 PM, Mickaël Salaün wrote: > 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" Ok, let's try this one. > > >> >>> >>> (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@huawei-partners.com> >>>> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> >>>> --- >>>> .../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. Good idea, thanks! > >> >>>> + >>>> +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. got it > >>>> + } else { >>>> + EXPECT_LE(0, fd) > > This should be an ASSERT because the following code using fd would make > no sense if executed. got it > >>>> + { >>>> + 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(). I mean we can use fixtures in order to not add ASSERT_EQ for each protocol, as suggested by Günther. It's gonna look like this: ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0)); ASSERT_EQ(EACCES, test_socket(&self->srv0)); I think it would make the test easier to read, don't you think so? > >> >> * 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. yeah, thanks > >> >>>> + >>>> +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. :) Yeah, thank you!)
On Thu, May 16, 2024 at 04:54:58PM +0300, Ivanov Mikhail wrote: > > > 5/8/2024 1:38 PM, Mickaël Salaün wrote: > > On Thu, Apr 11, 2024 at 06:58:34PM +0300, Ivanov Mikhail wrote: > > > > > > 4/8/2024 4:08 PM, Günther Noack wrote: > > > > > + { > > > > > + 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(). > > I mean we can use fixtures in order to not add ASSERT_EQ for > each protocol, as suggested by Günther. It's gonna look like this: > > ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0)); > ASSERT_EQ(EACCES, test_socket(&self->srv0)); > > I think it would make the test easier to read, don't you think so? Yes, this looks good.
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; +} + +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); + } else { + EXPECT_LE(0, fd) + { + TH_LOG("Failed to create socket: %s", strerror(errno)); + } + EXPECT_EQ(0, close(fd)); + } +} + +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)); + + enforce_ruleset(_metadata, ruleset_fd); + EXPECT_EQ(0, close(ruleset_fd)); + + test_socket_create(_metadata, &self->srv0, false); + + /* Denied create */ + ruleset_fd = landlock_create_ruleset(&ruleset_attr, + sizeof(ruleset_attr), 0); + ASSERT_LE(0, ruleset_fd); + + enforce_ruleset(_metadata, ruleset_fd); + EXPECT_EQ(0, close(ruleset_fd)); + + test_socket_create(_metadata, &self->srv0, true); +} + +TEST_HARNESS_MAIN