Message-ID: <9a8b71c6-f72c-7ec0-adee-5828dc41cf44@huawei-partners.com>
Date: Thu, 11 Apr 2024 18:58:34 +0300
From: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>
To: Günther Noack <gnoack@...gle.com>
CC: <mic@...ikod.net>, <willemdebruijn.kernel@...il.com>,
	<gnoack3000@...il.com>, <linux-security-module@...r.kernel.org>,
	<netdev@...r.kernel.org>, <netfilter-devel@...r.kernel.org>,
	<yusongping@...wei.com>, <artem.kuzin@...wei.com>,
	<konstantin.meskhidze@...wei.com>
Subject: Re: [RFC PATCH v1 03/10] selftests/landlock: Create 'create' test


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@...wei-partners.com>
>> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.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

Powered by blists - more mailing lists