netdev - Re: [RFC PATCH v1 03/10] selftests/landlock: Create 'create' test

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] [day] [month] [year] [list]

Date: Fri, 17 May 2024 17:24:41 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>
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

On Thu, May 16, 2024 at 04:54:58PM +0300, Ivanov Mikhail wrote:
> 
> 
> 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:


> > > > > +		{
> > > > > +			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?

Yes, this looks good.