netdev - Re: [RFC PATCH v4 10/15] seltest/landlock: add tests for bind() hooks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e1835aeb-8fe3-a8d7-9d36-69ce8e989291@huawei.com>
Date:   Wed, 6 Apr 2022 17:17:47 +0300
From:   Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
To:     Mickaël Salaün <mic@...ikod.net>
CC:     <willemdebruijn.kernel@...il.com>,
        <linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
        <netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
        <artem.kuzin@...wei.com>, <anton.sirazetdinov@...wei.com>
Subject: Re: [RFC PATCH v4 10/15] seltest/landlock: add tests for bind() hooks



4/4/2022 9:32 PM, Mickaël Salaün пишет:
> 
> On 09/03/2022 14:44, Konstantin Meskhidze wrote:
>> Adds two selftests for bind socket action.
>> The one is with no landlock restrictions:
>>      - bind_no_restrictions;
>> The second one is with mixed landlock rules:
>>      - bind_with_restrictions;
>>
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>> ---
>>
>> Changes since v3:
>> * Split commit.
>> * Add helper create_socket.
>> * Add FIXTURE_SETUP.
>>
>> ---
>>   .../testing/selftests/landlock/network_test.c | 153 ++++++++++++++++++
>>   1 file changed, 153 insertions(+)
>>   create mode 100644 tools/testing/selftests/landlock/network_test.c
>>
>> diff --git a/tools/testing/selftests/landlock/network_test.c 
>> b/tools/testing/selftests/landlock/network_test.c
>> new file mode 100644
>> index 000000000000..4c60f6d973a8
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/network_test.c
> 
> [...]
> 
>> +
>> +uint port[MAX_SOCKET_NUM];
>> +struct sockaddr_in addr[MAX_SOCKET_NUM];
> 
> You should not change global variables, it is a source of issue. Instead 
> use FIXTURE local variables accessible through self->X.
> 
   Sorry. Did not get your point here.
>> +
>> +const int one = 1;
> 
> This doesn't need to be global.

    Ok. Got it.
> 
> [...]
> 
>> +
>> +static void enforce_ruleset(struct __test_metadata *const _metadata,
>> +        const int ruleset_fd)
>> +{
>> +    ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
>> +    ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) {
>> +        TH_LOG("Failed to enforce ruleset: %s", strerror(errno));
>> +    }
>> +}
> 
> You should move the same helper from fs_base.c to common.h (see caps 
> helpers) and reuse it here.
> 
   Ok. Thanks.
> 
>> +
>> +FIXTURE(socket) { };
>> +
>> +FIXTURE_SETUP(socket)
>> +{
>> +    int i;
> 
> Please add a new line between declaration and actual code (everywhere).

   Ok. Got it. Will be refactored.
> 
>> +    /* Creates socket addresses */
>> +    for (i = 0; i < MAX_SOCKET_NUM; i++) {
> 
> Use ARRAY_SIZE() instead of MAY_SOCKET_NUM.
> 
   Ok. I got it.
> 
>> +        port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i;
> 
> Use self->port[i] and self->addr[i] instead.
> 

   Do you mean to add it in FIXTURE variables?

>> +        addr[i].sin_family = AF_INET;
>> +        addr[i].sin_port = htons(port[i]);
>> +        addr[i].sin_addr.s_addr = inet_addr(IP_ADDRESS);
>> +        memset(&(addr[i].sin_zero), '\0', 8);
>> +    }
>> +}
> 
> [...]
> 
>> +    /* Allows connect and deny bind operations to the port[1] socket. */
>> +    ASSERT_EQ(0, landlock_add_rule(ruleset_fd, 
>> LANDLOCK_RULE_NET_SERVICE,
>> +                &net_service_2, 0));
>> +    /* Empty allowed_access (i.e. deny rules) are ignored in network 
>> actions
> 
> The kernel coding style says to start a multi-line comments with a "/*" 
> and a new line.

   I missed it here. Thanks.
> .