Re: [RFC PATCH v1 03/10] selftests/landlock: Create 'create' test

["Günther Noack" <gnoack@google.com>]

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