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

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

quoted

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.

[...]

quoted

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

We can move this comment before the if and...

quoted

                        if (server_walker == client_walker)
                                return false;

                        return true;

...add this without the curly braces:

return server_walker != client_walker;

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.

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

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