[RFC PATCH v1 03/10] selftests/landlock: Create 'create' test
Ivanov Mikhail
ivanov.mikhail1 at huawei-partners.com
Thu May 16 13:54:58 UTC 2024
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 at huawei-partners.com>
>>>> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze at 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!)
More information about the Linux-security-module-archive
mailing list