netdev - Re: [RFC PATCH v2 04/12] selftests/landlock: Add protocol.socket_access

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]

Date: Thu, 30 May 2024 17:35:14 +0300
From: Mikhail Ivanov <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 v2 04/12] selftests/landlock: Add
 protocol.socket_access_rights to socket tests



5/27/2024 11:52 PM, Günther Noack wrote:
> Hello!
> 
> I see that this test is adapted from the network_access_rights test in
> net_test.c, and some of the subsequent are similarly copied from there.  It
> makes it hard to criticize the code, because being a little bit consistent is
> probably a good thing.  Have you found any opportunities to extract
> commonalities into common.h?

I think that all common tests should be extracted to common.h or maybe
some new header. *_test.c could maintain a fixture for these tests for
some rule-specific logic. Such refactoring should be in separate patch
though.

> 	
> On Fri, May 24, 2024 at 05:30:07PM +0800, Mikhail Ivanov wrote:
>> Add test that checks possibility of adding rule with every possible
>> access right.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
>> ---
>>
>> Changes since v1:
>> * Formats code with clang-format.
>> * Refactors commit message.
>> ---
>>   .../testing/selftests/landlock/socket_test.c  | 28 +++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> index 4c51f89ed578..eb5d62263460 100644
>> --- a/tools/testing/selftests/landlock/socket_test.c
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -178,4 +178,32 @@ TEST_F(protocol, create)
>>   	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
>>   }
>>   
>> +TEST_F(protocol, socket_access_rights)
>> +{
>> +	const struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_socket = ACCESS_ALL,
>> +	};
>> +	struct landlock_socket_attr protocol = {
>> +		.family = self->srv0.protocol.family,
>> +		.type = self->srv0.protocol.type,
>> +	};
>> +	int ruleset_fd;
>> +	__u64 access;
>> +
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +
>> +	for (access = 1; access <= ACCESS_LAST; access <<= 1) {
>> +		protocol.allowed_access = access;
>> +		EXPECT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +					       &protocol, 0))
>> +		{
>> +			TH_LOG("Failed to add rule with access 0x%llx: %s",
>> +			       access, strerror(errno));
>> +		}
>> +	}
>> +	EXPECT_EQ(0, close(ruleset_fd));
> 
> Reviewed-by: Günther Noack <gnoack@...gle.com>
> 
> P.S. We are inconsistent with our use of EXPECT/ASSERT for test teardown.  The
> fs_test.c uses ASSERT_EQ in these places whereas net_test.c and your new tests
> use EXPECT_EQ.
> 
> It admittedly does not make much of a difference for close(), so should be OK.
> Some other selftests are even ignoring the result for close().  If we want to
> make it consistent in the Landlock tests again, we can also do it in an
> independent sweep.
> 
> I filed a small cleanup task as a reminder:
> https://github.com/landlock-lsm/linux/issues/31
> 
> —Günther