Re: [PATCH v2 4/4] samples/landlock: Add POSIX message queue scoping to sandboxer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] landlock: user-triggerable WARN_ON_ONCE and wrong mount in hook_inode_create()
- [Medium] samples/landlock: commit message example with LL_FS_RW="" denies mq_open()
- [Medium] landlock: hook_inode_unlink() clears object->domain before unlink succeeds
- [Low] landlock: omitted parameter name in hook_inode_create() breaks GCC < 11 build
--

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

samples/landlock: Add POSIX message queue scoping to sandboxer

This series adds LANDLOCK_ACCESS_FS_MAKE_REG enforcement for POSIX message
queue creation, introduces LANDLOCK_SCOPE_POSIX_MSG_QUEUE in ABI 13, and
updates the Landlock sample sandboxer to support the new scope and audit
quieting option.

>     samples/landlock: Add POSIX message queue scoping to sandboxer
>     
>     Add "q" to LL_SCOPED to enable LANDLOCK_SCOPE_POSIX_MSG_QUEUE
>     and "posix_msg_queue" to LL_QUIET_ACCESS to quiet related audit
>     denials. Update the supported ABI version to 13 and disable the
>     new scope when running on older kernels.
>     
>     For example, on a kernel supporting Landlock ABI 13:
>     
>       LL_FS_RO="/" LL_FS_RW="" LL_SCOPED="q" \
>         ./samples/landlock/sandboxer /bin/sh
>     
>     The resulting shell and its children cannot open POSIX message
>     queues created outside their Landlock domain. They can create
>     queues and open queues created within that domain.
>     
>     Signed-off-by: Oxana Kharitonova <webprosto@gmail.com>

[Severity: Medium]
Does the example command in the commit message actually allow the shell and
its children to create or open POSIX message queues?

In samples/landlock/sandboxer.c:main(), ruleset_attr.handled_access_fs is
unconditionally initialized to access_fs_rw:

	__u64 access_fs_ro = ACCESS_FS_ROUGHLY_READ,
	      access_fs_rw = ACCESS_FS_ROUGHLY_READ | ACCESS_FS_ROUGHLY_WRITE;

	struct landlock_ruleset_attr ruleset_attr = {
		.handled_access_fs = access_fs_rw,

When LL_FS_RW="" is passed, populate_ruleset_fs() returns early without adding
any read-write rules:

	if (num_paths == 1 && path_list[0][0] == '\0') {
		/*
		 * Allows to not use all possible restrictions (e.g. use
		 * LL_FS_RO without LL_FS_RW).
		 */
		ret = 0;
		goto out_free_name;
	}

This leaves LANDLOCK_ACCESS_FS_MAKE_REG and LANDLOCK_ACCESS_FS_WRITE_FILE
handled and denied everywhere. Creating a queue via mq_open() with O_CREAT
invokes hook_inode_create(), which checks LANDLOCK_ACCESS_FS_MAKE_REG on
ipc_ns->mq_mnt and denies creation with EACCES.

In addition, do_mq_open() and hook_inode_create() operate on
current->nsproxy->ipc_ns->mq_mnt, which is an internal SB_KERNMOUNT mount
where path traversal in is_access_to_paths_allowed() stops at mq_mnt->mnt_root
(follow_up() returns false) and never reaches "/". Because of that,
LL_FS_RO="/" does not grant LANDLOCK_ACCESS_FS_READ_FILE to mq_open() either;
wouldn't the mqueue filesystem for the current IPC namespace (such as
/dev/mqueue) need to be mounted and included in LL_FS_RW for this example to
work as described?

> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 2ab67bcad1983..e9c52332dfcde 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
[ ... ]
> +/*
> + * 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: Medium]
Can clearing object->domain in hook_inode_unlink() strip the creator domain
from a queue before the unlink actually succeeds, or while the queue inode is
still alive?

In vfs_unlink(), security_inode_unlink(dir, dentry) is a pre-operation
permission hook invoked before try_break_deleg() and dir->i_op->unlink().
Because Landlock is ordered before SELinux, Smack, AppArmor, and BPF LSM in
CONFIG_LSM, hook_inode_unlink() runs first, sets object->domain = NULL, and
drops the domain and object references retained by hook_d_instantiate().

If a subsequent stacked LSM hook or delegation break fails, vfs_unlink()
aborts and the queue remains linked in mqueuefs with object->domain == NULL.
Any later mq_open() or open() from the creator's domain with
LANDLOCK_SCOPE_POSIX_MSG_QUEUE enabled will see creator_domain == NULL in
landlock_check_posix_mqueue_open() and fail with -EPERM. The same -EPERM also
occurs if an unlinked queue with an open file descriptor is reopened by the
creator's domain via /proc/self/fd/<fd>.

Should the domain reference instead be released when the underlying inode is
evicted or released rather than inside the pre-unlink permission hook?

[ ... ]
> +/*
> + * Unlike regular file creation, mq_open(2) calls vfs_mkobj() directly and
> + * therefore does not trigger the path_mknod hook.  Use the mqueuefs mount
> + * associated with the current IPC namespace to recover the parent path.
> + */
> +static int hook_inode_create(struct inode *const dir,
> +			     struct dentry *const dentry, const umode_t)

[Severity: Low]
Does omitting the parameter name for const umode_t in hook_inode_create()
break builds with older supported GCC versions?

The kernel compiles C sources with -std=gnu11 and supports GCC >= 8.1.0,
whereas unnamed parameters in function definitions are a C23 feature only
supported by GCC 11 and later (older versions fail with "error: parameter name
omitted").

> +{
> +	struct ipc_namespace *ipc_ns;
> +	struct vfsmount *mnt;
> +	struct path parent;
> +
> +	if (!landlock_is_posix_mqueue_fs(dir))
> +		return 0;
> +
> +	if (!current->nsproxy)
> +		return 0;
> +	ipc_ns = current->nsproxy->ipc_ns;
> +	if (!ipc_ns)
> +		return 0;
> +	mnt = ipc_ns->mq_mnt;
> +	if (WARN_ON_ONCE(!mnt || mnt->mnt_sb != dir->i_sb))
> +		return 0;
> +
> +	parent.mnt = mnt;
> +	parent.dentry = dentry->d_parent;
> +	return current_check_access_path(&parent, LANDLOCK_ACCESS_FS_MAKE_REG);
> +}

[Severity: High]
Can hook_inode_create() also be reached via VFS path-based creation on a
mounted mqueue filesystem, triggering the WARN_ON_ONCE() from unprivileged
userspace or checking the wrong mount?

In ipc/mqueue.c, mqueue_dir_inode_operations sets .create = mqueue_create, so
calling open(..., O_CREAT, ...) or mknod(..., S_IFREG, ...) on a mounted
mqueue filesystem (such as /dev/mqueue, which has mode 01777) calls
security_path_mknod() followed by security_inode_create() ->
hook_inode_create().

This appears to cause two regressions:

1. A mounted mqueue superblock belongs to the ipc_namespace active at mount
time (sb->s_fs_info), which need not match current->nsproxy->ipc_ns. An
unprivileged user can call unshare(CLONE_NEWUSER | CLONE_NEWIPC) while keeping
access to the previous IPC namespace's /dev/mqueue mount and call
open("/dev/mqueue/foo", O_CREAT, 0600). In hook_inode_create(),
current->nsproxy->ipc_ns->mq_mnt->mnt_sb != dir->i_sb, triggering the
WARN_ON_ONCE() and returning 0 without enforcing LANDLOCK_ACCESS_FS_MAKE_REG.

2. Even within the same IPC namespace, if a sandboxed process creates a queue
via open("/dev/mqueue/foo", O_CREAT, 0600) or mknod() and its ruleset grants
LANDLOCK_ACCESS_FS_MAKE_REG on an ancestor directory such as "/" or "/dev"
rather than directly on the /dev/mqueue root inode, hook_path_mknod() succeeds
by walking up across the mountpoint via follow_up(). However,
hook_inode_create() then checks parent.mnt = ipc_ns->mq_mnt (an internal
SB_KERNMOUNT mount where follow_up() stops at the root) and denies the
creation with -EACCES.

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