Re: [PATCH v2 3/4] Landlock: Add POSIX message queue scoping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] landlock: premature creator domain clearing in hook_inode_unlink()
- [Medium] landlock: ignored allocation failure in hook_d_instantiate() leaves queue untagged
- [Medium] selftests/landlock: missing ENOSYS skip checks when CONFIG_POSIX_MQUEUE=n
- [Low] landlock: build failure in landlock_check_posix_mqueue_open() without tracepoints
- [Low] Documentation: landlock: section title underline too short in landlock.rst
--

commit 672099a186033eeb6307163bb1baca6370e0b662
Author: Oxana Kharitonova <webprosto@gmail.com>

Landlock: Add POSIX message queue scoping

This commit introduces Posix message queue scoping in Landlock (ABI 13) to
restrict opening queues to those created within the scoping domain or its
descendants. It also adds audit logging, a denial trace event, documentation,
and selftests.

> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 1fb91884cb8af..fa65fa4aad02e 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst

[ ... ]

> @@ -1075,6 +1121,14 @@ fd-reference acquisition (:manpage:`open_tree(2)`, :manpage:`fsmount(2)`) with
>  the new ``LANDLOCK_PERMISSION_NAMESPACE_USE`` permission flag and
>  ``LANDLOCK_RULE_NAMESPACE`` rule type.
>  
> +POSIX message queue scoping (ABI < 13)
> +-------------------------------------

[Severity: Low]
This isn't a bug, but the hyphen underline here is 37 characters while the
heading above it is 38 characters. Should an extra hyphen be added so htmldocs
builds do not emit a title underline warning?

> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index d5dcb19d5eb19..e9c52332dfcde 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -53,6 +53,7 @@
>  #include "object.h"
>  #include "ruleset.h"
>  #include "setup.h"
> +#include "task.h"
>  
>  #include <trace/events/landlock.h>
>  
> @@ -1374,6 +1375,80 @@ static void hook_inode_free_security_rcu(void *inode_security)
>  	WARN_ON_ONCE(inode_sec->object);
>  }
>  
> +/*
> + * Tag a newly created POSIX message queue with its creator's domain.
> + *
> + * This is the earliest reachable point with both the creator's context and a
> + * fully initialized inode.
> + */
> +static void hook_d_instantiate(struct dentry *const dentry,
> +			       struct inode *const inode)
> +{
> +	struct landlock_domain *dom;
> +	struct landlock_object *object;
> +
> +	if (!landlock_is_posix_mqueue_inode(inode))
> +		return;
> +
> +	dom = landlock_get_current_domain();
> +	if (!dom)
> +		return;
> +
> +	object = get_inode_object(inode);
> +	if (IS_ERR(object))
> +		return;

[Severity: Medium]
What happens if get_inode_object(inode) fails to allocate memory in
hook_d_instantiate() when a task in a scoped domain creates a queue via
mq_open()?

Because security_d_instantiate() is a void hook called from:

    mqueue_create_attr()
      d_make_persistent()
        security_d_instantiate()
          hook_d_instantiate()

the allocation failure is ignored and the new queue is linked into mqueuefs
with no creator domain attached. Immediately afterwards, mqueue_file_open()
calls dentry_open() -> hook_file_open() -> landlock_check_posix_mqueue_open(),
which sees creator_domain == NULL, treats the queue as created outside any
Landlock domain, and denies the creator's own mq_open() with EPERM while
leaving the untagged queue behind in the namespace.

Could the object allocation or tagging be performed in a fallible creation
hook (or preallocated before d_instantiate) so that allocation failure aborts
queue creation cleanly instead of leaving an untagged queue?

> +
> +	spin_lock(&object->lock);
> +	if (WARN_ON_ONCE(object->domain)) {
> +		spin_unlock(&object->lock);
> +		landlock_put_object(object);
> +		return;
> +	}
> +
> +	landlock_get_domain(dom);
> +	object->domain = dom;
> +	spin_unlock(&object->lock);
> +}
> +
> +/*
> + * Drop the reference retained by hook_d_instantiate() for a POSIX message
> + * queue once its sole name is unlinked.
> + */
> +static int hook_inode_unlink(struct inode *const dir,
> +			     struct dentry *const dentry)
> +{
> +	const struct inode *const inode = d_backing_inode(dentry);
> +	struct landlock_domain *domain;
> +	struct landlock_object *object;
> +
> +	if (!landlock_is_posix_mqueue_inode(inode))
> +		return 0;
> +
> +	rcu_read_lock();
> +	object = rcu_dereference(landlock_inode(inode)->object);
> +	/* The temporary reference protects against concurrent object release. */
> +	if (!object || !refcount_inc_not_zero(&object->usage)) {
> +		rcu_read_unlock();
> +		return 0;
> +	}
> +	rcu_read_unlock();
> +
> +	spin_lock(&object->lock);
> +	domain = object->domain;
> +	object->domain = NULL;
> +	spin_unlock(&object->lock);
> +
> +	/* Drop the temporary reference acquired by refcount_inc_not_zero(). */
> +	landlock_put_object(object);
> +	if (domain) {
> +		landlock_put_domain_deferred(domain);
> +		/* Drops the reference retained by hook_d_instantiate(). */
> +		landlock_put_object(object);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Can clearing object->domain inside hook_inode_unlink() strip the creator
domain from a queue that remains linked or open?

In fs/namei.c:vfs_unlink(), security_inode_unlink() is a pre-unlink
permission check:

    vfs_unlink()
      security_inode_unlink(dir, dentry)
      try_break_deleg(dir, ...)
      try_break_deleg(target, ...)
      dir->i_op->unlink(dir, dentry)

If Landlock runs before another stacked security module (such as SELinux,
AppArmor, Smack, or BPF LSM in the default lsm order) and that subsequent
module denies the unlink, or if try_break_deleg() fails, hook_inode_unlink()
has already cleared object->domain = NULL and dropped the retained references,
even though vfs_unlink() aborts and the queue remains linked in mqueuefs.

Similarly, even when vfs_unlink() succeeds, the queue inode stays alive while
open file descriptors exist and can be reopened by tasks in the creator's
domain via /proc/<pid>/fd/<n>.

Because landlock_check_posix_mqueue_open() treats creator_domain == NULL as
an unsandboxed creator, any subsequent open of that queue by the creator's
own scoped domain (or a scoped ancestor domain) will be denied with EPERM.
Should the creator domain stay attached for the lifetime of the inode rather
than being cleared in hook_inode_unlink()?

> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index d9eae86fc5529..0daa7edff2d7e 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -522,6 +522,72 @@ static int hook_file_send_sigiotask(struct task_struct *tsk,
>  	return -EPERM;
>  }
>  
> +static const struct access_masks posix_mqueue_scope = {
> +	.scope = LANDLOCK_SCOPE_POSIX_MSG_QUEUE,
> +};
> +
> +/**
> + * landlock_check_posix_mqueue_open - Deny opening a POSIX message queue
> + * created by a task from a different (non-ancestor) domain
> + *
> + * @file: The mqueuefs file being opened.
> + *
> + * Return: -EPERM if the open must be denied, 0 otherwise.
> + */
> +int landlock_check_posix_mqueue_open(struct file *const file)
> +{

[ ... ]

> +	landlock_log_denial(subject, &(struct landlock_request) {
> +		.type = LANDLOCK_REQUEST_SCOPE_POSIX_MSG_QUEUE,
> +		.other_domain_id = creator_domain ? creator_domain->hierarchy->id : 0,
> +		.audit = {
> +			.type = LSM_AUDIT_DATA_FILE,
> +			.u.file = file,
> +		},
> +		.layer_plus_one = handle_layer + 1,
> +	});

[Severity: Low]
Does security/landlock/task.c build when tracepoints are disabled?

In security/landlock/log.h, other_domain_id in struct landlock_request is
guarded by:

    #ifdef CONFIG_TRACEPOINTS
    	union {
    		u64 other_domain_id;
    ...

and in security/landlock/domain.h, creator_domain->hierarchy->id is guarded by
#ifdef CONFIG_SECURITY_LANDLOCK_LOG. Because landlock_log_denial() is a static
inline function when logging is disabled, the compound literal is still
type-checked by the compiler. Should the .other_domain_id initializer here be
wrapped in #ifdef CONFIG_TRACEPOINTS like in hook_unix_stream_connect() and
hook_unix_may_send()?

> diff --git a/tools/testing/selftests/landlock/audit_test.c b/tools/testing/selftests/landlock/audit_test.c
> index 4bea8c880a4d3..a15c1e5d277f3 100644
> --- a/tools/testing/selftests/landlock/audit_test.c
> +++ b/tools/testing/selftests/landlock/audit_test.c

[ ... ]

> +/* Checks audit logging and quieting of a denied POSIX message queue open. */
> +TEST_F(posix_mqueue_audit, open)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.scoped = LANDLOCK_SCOPE_POSIX_MSG_QUEUE,
> +		.quiet_scoped = variant->quiet_scoped,
> +	};
> +	char mq_name[NAME_MAX], log_match[PATH_MAX];
> +	int pipe_parent[2], pipe_child[2], ruleset_fd, status;
> +	struct audit_records records;
> +	char buf;
> +	pid_t child;
> +	mqd_t mq;
> +
> +	ASSERT_LT(0,
> +		  snprintf(mq_name, sizeof(mq_name),
> +			   "/selftests-landlock-audit-mq-tid%d", sys_gettid()));
> +	mq_unlink(mq_name);
> +	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
> +	ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
> +
> +	child = fork();
> +	ASSERT_LE(0, child);
> +	if (child == 0) {
> +		EXPECT_EQ(0, close(pipe_parent[1]));
> +		EXPECT_EQ(0, close(pipe_child[0]));
> +
> +		mq = mq_open(mq_name, O_CREAT | O_EXCL | O_RDWR, 0600, NULL);
> +		ASSERT_LE(0, mq);

[ ... ]

> diff --git a/tools/testing/selftests/landlock/scoped_posix_msg_queue_test.c b/tools/testing/selftests/landlock/scoped_posix_msg_queue_test.c
> new file mode 100644
> index 0000000000000..4475746081bc3
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/scoped_posix_msg_queue_test.c

[ ... ]

> +	if (variant->domain_parent)
> +		create_scoped_domain(_metadata, LANDLOCK_SCOPE_POSIX_MSG_QUEUE);
> +
> +	mq = mq_open(self->mq_name, O_CREAT | O_RDWR, 0600, NULL);
> +	ASSERT_LE(0, mq);

[Severity: Medium]
In tools/testing/selftests/landlock/scoped_posix_msg_queue_test.c, the later
tests check whether mq_open() failed with errno == ENOSYS and call SKIP() when
Posix message queue support is not enabled in the kernel, but the first eight
tests (open_parent_queue, open_child_queue, open_existing_queue_with_create,
unlink_and_recreate_in_sibling_domain, open_queue_after_creator_exits,
open_queue_from_child_after_creator_exits, posix_mqueue_ipc_namespace_lifetime,
and create_and_open_same_domain) along with TEST_F(posix_mqueue_audit, open)
in tools/testing/selftests/landlock/audit_test.c assert that mq_open()
succeeds directly.

Should those tests also skip when mq_open() fails with ENOSYS so they do not
fail on kernels built without Posix message queue support?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102144708.920232-1-webprosto@gmail.com?part=3