Message-ID: <986f11a9-1426-a87d-c43e-a86380305a21@huawei-partners.com>
Date: Thu, 16 May 2024 16:54:58 +0300
From: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: Günther Noack <gnoack@...gle.com>,
	<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



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@...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.
> 
> 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!)

Powered by blists - more mailing lists