Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

On Thu, Aug 08, 2024 at 05:17:10PM -0600, Tahera Fahimi wrote:
> On Wed, Aug 07, 2024 at 04:44:36PM +0200, Mickaël Salaün wrote:
> > On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> > > On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <[email protected]> wrote:
> > > > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > > > I think adding something like this change on top of your code would
> > > > > make it more concise (though this is entirely untested):
> > > > >
> > > > > --- /tmp/a      2024-08-06 22:37:33.800158308 +0200
> > > > > +++ /tmp/b      2024-08-06 22:44:49.539314039 +0200
> > > > > @@ -15,25 +15,12 @@
> > > > >           * client_layer must be a signed integer with greater capacity than
> > > > >           * client->num_layers to ensure the following loop stops.
> > > > >           */
> > > > >          BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > > >
> > > > > -        if (!server) {
> > > > > -                /*
> > > > > -                 * Walks client's parent domains and checks that none of these
> > > > > -                 * domains are scoped.
> > > > > -                 */
> > > > > -                for (; client_layer >= 0; client_layer--) {
> > > > > -                        if (landlock_get_scope_mask(client, client_layer) &
> > > > > -                            scope)
> > > > > -                                return true;
> > > > > -                }
> > > > > -                return false;
> > > > > -        }
> > > >
> > > > This loop is redundant with the following one, but it makes sure there
> > > > is no issue nor inconsistencies with the server or server_walker
> > > > pointers.  That's the only approach I found to make sure we don't go
> > > > through a path that could use an incorrect pointer, and makes the code
> > > > easy to review.
> > > 
> > > My view is that this is a duplication of logic for one particular
> > > special case - after all, you can also end up walking up to the same
> > > state (client_layer==-1, server_layer==-1, client_walker==NULL,
> > > server_walker==NULL) with the loop at the bottom.
> > 
> > Indeed
> > 
> > > 
> > > But I guess my preference for more concise code is kinda subjective -
> > > if you prefer the more verbose version, I'm fine with that too.
> > > 
> > > > > -
> > > > > -        server_layer = server->num_layers - 1;
> > > > > -        server_walker = server->hierarchy;
> > > > > +        server_layer = server ? (server->num_layers - 1) : -1;
> > > > > +        server_walker = server ? server->hierarchy : NULL;
> > > >
> > > > We would need to change the last loop to avoid a null pointer deref.
> > > 
> > > Why? The first loop would either exit or walk the client_walker up
> > > until client_layer is -1 and client_walker is NULL; the second loop
> > > wouldn't do anything because the walkers are at the same layer; the
> > > third loop's body wouldn't be executed because client_layer is -1.
> > 
> > Correct, I missed that client_layer would always be greater than
> > server_layer (-1).
> > 
> > Tahera, could you please take Jann's proposal?
> Done.
> We will have duplicate logic, but it would be easier to read and review.

With Jann's proposal we don't have duplicate logic.

> > 
> > > 
> > > The case where the server is not in any Landlock domain is just one
> > > subcase of the more general case "client and server do not have a
> > > common ancestor domain".
> > > 
> > > > >
> > > > >          /*
> > > > >           * Walks client's parent domains down to the same hierarchy level as
> > > > >           * the server's domain, and checks that none of these client's parent
> > > > >           * domains are scoped.
> > > > >
> > > 
>