On Tue, Aug 6, 2024 at 9:36 PM Mickaël Salaün [off-list ref] wrote:

On Sat, Aug 03, 2024 at 01:29:09PM +0200, Mickaël Salaün wrote:

quoted

On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:

quoted

This patch introduces a new "scoped" attribute to the landlock_ruleset_attr
that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope
abstract Unix sockets from connecting to a process outside of
the same landlock domain. It implements two hooks, unix_stream_connect
and unix_may_send to enforce this restriction.

[...]

Here is a refactoring that is easier to read and avoid potential pointer
misuse:

static bool domain_is_scoped(const struct landlock_ruleset *const client,
                             const struct landlock_ruleset *const server,
                             access_mask_t scope)
{
        int client_layer, server_layer;
        struct landlock_hierarchy *client_walker, *server_walker;

        if (WARN_ON_ONCE(!client))
                return false;

        client_layer = client->num_layers - 1;
        client_walker = client->hierarchy;

        /*
         * 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;
        }

        server_layer = server->num_layers - 1;
        server_walker = server->hierarchy;

        /*
         * 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.
         */
        for (; client_layer > server_layer; client_layer--) {
                if (landlock_get_scope_mask(client, client_layer) & scope)
                        return true;

                client_walker = client_walker->parent;
        }

        /*
         * Walks server's parent domains down to the same hierarchy level as
         * the client's domain.
         */
        for (; server_layer > client_layer; server_layer--)
                server_walker = server_walker->parent;

        for (; client_layer >= 0; client_layer--) {
                if (landlock_get_scope_mask(client, client_layer) & scope) {
                        /*
                         * Client and server are at the same level in the
                         * hierarchy.  If the client is scoped, the request is
                         * only allowed if this domain is also a server's
                         * ancestor.
                         */
                        if (server_walker == client_walker)
                                return false;

                        return true;
                }
                client_walker = client_walker->parent;
                server_walker = server_walker->parent;
        }
        return false;
}

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;
-        }
-
-        server_layer = server->num_layers - 1;
-        server_walker = server->hierarchy;
+        server_layer = server ? (server->num_layers - 1) : -1;
+        server_walker = server ? server->hierarchy : NULL;

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