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@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.
> +
> +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
