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:

quoted

On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün [off-list ref] wrote:

quoted

On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:

quoted

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

quoted

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.

quoted

quoted

-
-        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.

quoted

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".

quoted

quoted

         /*
          * 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.