Re: [PATCH v13 08/12] landlock: Add network rules and TCP hooks support

["Mickaël Salaün" <mic@digikod.net>]

On Fri, Oct 20, 2023 at 02:58:31PM +0300, Konstantin Meskhidze (A) wrote:
> 
> 
> 10/20/2023 12:49 PM, Mickaël Salaün пишет:
> > On Fri, Oct 20, 2023 at 07:08:33AM +0300, Konstantin Meskhidze (A) wrote:
> > > 
> > > 
> > > 10/18/2023 3:29 PM, Mickaël Salaün пишет:
> > > > On Mon, Oct 16, 2023 at 09:50:26AM +0800, Konstantin Meskhidze wrote:

> > > > > diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> > > > > index 4c209acee01e..1fe4298ff4a7 100644
> > > > > --- a/security/landlock/ruleset.c
> > > > > +++ b/security/landlock/ruleset.c
> > > > > @@ -36,6 +36,11 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
> > > > >  	refcount_set(&new_ruleset->usage, 1);
> > > > >  	mutex_init(&new_ruleset->lock);
> > > > >  	new_ruleset->root_inode = RB_ROOT;
> > > > > +
> > > > > +#if IS_ENABLED(CONFIG_INET)
> > > > > +	new_ruleset->root_net_port = RB_ROOT;
> > > > > +#endif /* IS_ENABLED(CONFIG_INET) */
> > > > > +
> > > > >  	new_ruleset->num_layers = num_layers;
> > > > >  	/*
> > > > >  	 * hierarchy = NULL
> > > > > @@ -46,16 +51,21 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
> > > > >  }
> > > > > > >  struct landlock_ruleset *
> > > > > -landlock_create_ruleset(const access_mask_t fs_access_mask)
> > > > > +landlock_create_ruleset(const access_mask_t fs_access_mask,
> > > > > +			const access_mask_t net_access_mask)
> > > > >  {
> > > > >  	struct landlock_ruleset *new_ruleset;
> > > > > > >  	/* Informs about useless ruleset. */
> > > > > -	if (!fs_access_mask)
> > > > > +	if (!fs_access_mask && !net_access_mask)
> > > > >  		return ERR_PTR(-ENOMSG);
> > > > >  	new_ruleset = create_ruleset(1);
> > > > > -	if (!IS_ERR(new_ruleset))
> > > > > +	if (IS_ERR(new_ruleset))
> > > > > +		return new_ruleset;
> > > > > +	if (fs_access_mask)
> > > > >  		landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
> > > > > +	if (net_access_mask)
> > > > > +		landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
> > > > > This is good, but it is not tested: we need to add a test that
> > > both
> > > > handle FS and net restrictions. You can add one in net.c, just handling
> > > > LANDLOCK_ACCESS_FS_READ_DIR and LANDLOCK_ACCESS_NET_BIND_TCP, add one
> > > > rule with path_beneath (e.g. /dev) and another with net_port, and check
> > > > that open("/") is denied, open("/dev") is allowed, and and only the
> > > > allowed port is allowed with bind(). This test should be simple and can
> > > > only check against an IPv4 socket, i.e. using ipv4_tcp fixture, just
> > > > after port_endianness. fcntl.h should then be included by net.c
> > > 
> > >   Ok.
> > > > > I guess that was the purpose of layout1.with_net (in fs_test.c)
> > > but it
> > > 
> > >   Yep. I added this kind of nest in fs_test.c to test both fs and network
> > > rules together.
> > > > is not complete. You can revamp this test and move it to net.c
> > > > following the above suggestions, keeping it consistent with other tests
> > > > in net.c . You don't need the test_open() nor create_ruleset() helpers.
> > > > > This test must failed if we change
> > > "ruleset->access_masks[layer_level] |="
> > > > to "ruleset->access_masks[layer_level] =" in
> > > > landlock_add_fs_access_mask() or landlock_add_net_access_mask().
> > > 
> > >   Do you want to change it? Why?
> > 
> > The kernel code is correct and must not be changed. However, if by
> > mistake we change it and remove the OR, a test should catch that. We
> > need a test to assert this assumption.
> > 
>   OK. I will add additional assert simulating
> "ruleset->access_masks[layer_level] =" kernel code.
> > >   Fs and network masks are ORed to not intersect with each other.
> > 
> > Yes, they are ORed, and we need a test to check that. Noting is
> > currently testing this OR (and the different rule type consistency).
> > I'm suggesting to revamp the layout1.with_net test into
> > ipv4_tcp.with_fs and make it check ruleset->access_masks[] and rule
> > addition of different types.
> 
>   I will move layout1.with_net test into net.c and rename it. Looks like
>   it just needed to add "ruleset->access_masks[layer_level] =" assert
>   because the test already has rule addition with different types.

The with_net test doesn't have FS rules, which is the main missing part.
You'll need to rely on the net.c helpers, use the hardcoded paths, and
only handle one access right of each type as I suggested above.

> 
>   Do you have any more review updates so far?

That's all for this patch series. :)