Re: [PATCH RFC 2/3] landlock: Add access rights on created files

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] landlock: use-after-free in landlock_unmask_created() during path walk
- [High] landlock: uninitialized layer_masks read in refer_created()
- [High] landlock: refer_created() mutates state and pins inodes before VFS checks
- [High] landlock: hook_inode_init_security() pins I_NEW inodes without error rollback
- [High] landlock: GFP_KERNEL_ACCOUNT allocation in hook_inode_init_security()
- [Medium] landlock: grant_created() miscomputes reparented access rights
- [Medium] landlock: missing hook_inode_init_security() calls and unlinked inode pinning
--

commit 5e3bc0f81c353fca29a705bd4d2b123bb4da9434
Author: Farid Zakaria <farid.m.zakaria@gmail.com>

landlock: Add access rights on created files

This commit adds the LANDLOCK_ADD_RULE_CREATED flag for
LANDLOCK_RULE_PATH_BENEATH rules on directories so that access rights are
granted on file hierarchies created directly in the directory by tasks in the
domain (or nested domains), and bumps the Landlock ABI version to 13.

> diff --git a/security/landlock/domain.c b/security/landlock/domain.c
> index 4c0b1f0e1db5d..f8dc771724e14 100644
> --- a/security/landlock/domain.c
> +++ b/security/landlock/domain.c

[ ... ]

> @@ -198,6 +199,166 @@ bool landlock_unmask_layers(const struct landlock_domain *const domain,

[ ... ]

> +bool landlock_unmask_created(const struct landlock_domain *const domain,
> +			     const struct landlock_object *const object,
> +			     struct layer_masks *masks)
> +{
> +	struct landlock_hierarchy *hierarchy = domain->hierarchy;
> +
> +	if (!masks)
> +		return true;
> +
> +	if (!READ_ONCE(object->created))
> +		return false;
> +
> +	for (size_t i = domain->num_layers; i > 0 && hierarchy;
> +	     i--, hierarchy = hierarchy->parent)
> +		masks->layers[i - 1].access &= ~get_created(hierarchy, object);
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(masks->layers); i++) {
> +		if (masks->layers[i].access)
> +			return false;
> +	}
> +	return true;
> +}

[Severity: Critical]
Can this dereference of object->created in landlock_unmask_created() trigger a
use-after-free or grant access to the wrong inode via slab pointer reuse?

During the sleepable path walks in is_access_to_paths_allowed() and
collect_domain_accesses() in security/landlock/fs.c, get_inode_id() fetches
the raw object pointer without holding rcu_read_lock() or taking a reference:

security/landlock/fs.c:get_inode_id() {
	...
	id->key.object = rcu_access_pointer(
		landlock_inode(d_backing_inode(dentry))->object);
	return !!id->key.object;
}

Previously, id.key.object was never dereferenced and was only compared
numerically against domain->rules.root_inode, which is immutable and pins its
objects for the lifetime of the domain.

With unmask_layers_created() passing id.key.object to
landlock_unmask_created(), if an inode visited during the path walk has a
landlock_object attached by another ruleset or domain that is concurrently
freed via landlock_put_object() -> kfree_rcu(object, rcu_free), the RCU grace
period can elapse while the path walk is running. Could READ_ONCE(object->created)
then read from freed slab memory?

Furthermore, because hierarchy->created is updated at runtime by
landlock_add_created(), if that freed landlock_object slab slot is reallocated
for a newly created inode in this domain and inserted into hierarchy->created,
would get_created(hierarchy, object) match the reused pointer address and
grant the created inode's rights to the unrelated inode?

> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 2ab67bcad1983..64a3447254337 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c

[ ... ]

> @@ -1359,8 +1381,130 @@ static int current_check_refer_path(struct dentry *const old_dentry,

[ ... ]

> +static int grant_created(const struct landlock_domain *const domain,
> +			 const struct landlock_rule *const rule,
> +			 struct inode *const inode,
> +			 const struct layer_masks *const src_masks)
> +{
> +	struct landlock_object *object;
> +	int err = 0;
> +
> +	object = get_inode_object(inode);
> +	if (IS_ERR(object))
> +		return PTR_ERR(object);
> +
> +	for (size_t i = 0; i < rule->num_layers && !err; i++) {
> +		const struct landlock_layer *const layer = &rule->layers[i];
> +
> +		if (src_masks &&
> +		    (src_masks->layers[layer->level - 1].access & layer->access))
> +			continue;
> +
> +		err = landlock_add_created(domain, layer->level - 1, object,
> +					   layer->access);
> +	}
> +	landlock_put_object(object);
> +	return err;
> +}

[Severity: Medium]
When reparenting a file or directory via refer_created(), does grant_created()
drop the entire layer whenever any single bit of layer->access was not granted
at the source?

In refer_created(), layer_masks (passed as src_masks) holds the access bits
from rule that were unfulfilled at old_path. Checking:

	if (src_masks &&
	    (src_masks->layers[layer->level - 1].access & layer->access))
		continue;

skips the entire layer whenever any bit in layer->access remains unfulfilled
at the source (for instance, when created_attr.allowed_access is set to
ruleset_attr.handled_access_fs as shown in the documentation, but the source
staging directory did not grant LANDLOCK_ACCESS_FS_MAKE_BLOCK or
LANDLOCK_ACCESS_FS_IOCTL_DEV), rather than granting the intersection of rights
that were already allowed at the source
(layer->access & ~src_masks->layers[layer->level - 1].access).

Also, when old_dentry is not a directory, refer_created() masks
access &= ACCESS_FILE before calling landlock_init_layer_masks(), leaving
non-ACCESS_FILE bits zeroed in src_masks, so grant_created() passes the
unmasked layer->access (including directory access rights) to
landlock_add_created(). Should this grant only the fulfilled subset of
applicable rights instead?

> +
> +/*
> + * Grants the access rights of @new_dir's LANDLOCK_ADD_RULE_CREATED rule on
> + * the reparented @old_dentry, for the layers which already granted them at
> + * its source.  This must only be called once the refer action is allowed.
> + */
> +static int refer_created(struct dentry *const old_dentry,
> +			 const struct path *const new_dir)
> +{
> +	const struct landlock_cred_security *const subject =
> +		landlock_get_applicable_subject(current_cred(), any_fs, NULL);
> +	/* The mount points are the same for old and new paths, cf. EXDEV. */
> +	const struct path old_path = {
> +		.mnt = new_dir->mnt,
> +		.dentry = old_dentry,
> +	};
> +	const struct landlock_rule *rule;
> +	struct layer_masks layer_masks;
> +	access_mask_t access = 0;
> +
> +	if (!subject ||
> +	    RB_EMPTY_ROOT(&subject->domain->rules.root_inode_created) ||
> +	    old_dentry->d_parent == new_dir->dentry)
> +		return 0;
> +
> +	rule = get_created_rule(subject->domain,
> +				d_backing_inode(new_dir->dentry));
> +	if (!rule)
> +		return 0;
> +
> +	for (size_t i = 0; i < rule->num_layers; i++)
> +		access |= rule->layers[i].access;
> +
> +	/* Only compares the access rights which make sense for files. */
> +	if (!d_is_dir(old_dentry))
> +		access &= ACCESS_FILE;
> +
> +	access = landlock_init_layer_masks(subject->domain, access,
> +					   &layer_masks, LANDLOCK_KEY_INODE);
> +	is_access_to_paths_allowed(subject->domain, &old_path, access,
> +				   &layer_masks, NULL, NULL, 0, NULL, NULL,
> +				   NULL);
> +	return grant_created(subject->domain, rule,
> +			     d_backing_inode(old_dentry), &layer_masks);
> +}

[Severity: High]
Can refer_created() pass an uninitialized layer_masks struct on the stack to
grant_created()?

When old_dentry is a non-directory (!d_is_dir(old_dentry)), access is masked
with ACCESS_FILE. If the ruleset handles all ACCESS_FILE rights and the
LANDLOCK_ADD_RULE_CREATED rule on new_dir only grants directory rights (such
as LANDLOCK_ACCESS_FS_READ_DIR | LANDLOCK_ACCESS_FS_MAKE_REG), access becomes
0 after access &= ACCESS_FILE.

When access is 0, landlock_init_layer_masks() in security/landlock/domain.c
returns early without initializing layer_masks:

	if (!access_request)
		return 0;

and is_access_to_paths_allowed() in security/landlock/fs.c also returns early
without writing to layer_masks:

	if (!access_request_parent1 && !access_request_parent2)
		return true;

Wouldn't grant_created() then read uninitialized stack memory from
src_masks->layers[layer->level - 1].access?

> +
> +/* Inode hooks */
> +
> +static int hook_inode_init_security(struct inode *const inode,
> +				    struct inode *const dir,
> +				    const struct qstr *const qstr,
> +				    struct xattr *const xattrs,
> +				    int *const xattr_count)
> +{
> +	const struct landlock_cred_security *const subject =
> +		landlock_get_applicable_subject(current_cred(), any_fs, NULL);
> +	const struct landlock_rule *rule;
> +	int err;
> +
> +	if (!subject || !dir ||
> +	    RB_EMPTY_ROOT(&subject->domain->rules.root_inode_created))
> +		return -EOPNOTSUPP;
> +
> +	rule = get_created_rule(subject->domain, dir);
> +	if (!rule)
> +		return -EOPNOTSUPP;
> +
> +	/* Landlock does not provide any xattr. */
> +	err = grant_created(subject->domain, rule, inode, NULL);
> +	return err ?: -EOPNOTSUPP;
> +}

[Severity: High]
What happens to the newly allocated I_NEW | I_CREATING inode if creation fails
after hook_inode_init_security() pins it in hierarchy->created?

In hook_inode_init_security(), grant_created() calls get_inode_object(inode)
(which calls ihold(inode), incrementing i_count from 1 to 2) and
landlock_add_created() (which holds a reference to object in
hierarchy->created until the domain hierarchy is freed).

If inode creation subsequently fails — for example, in grant_created() itself
when layer 0 succeeds and a subsequent layer fails with -ENOMEM, in a later
LSM hook or initxattrs() callback inside security_inode_init_security(), when
the filesystem fails to add the directory entry (e.g. -ENOSPC, -EDQUOT,
-EMLINK, -ENOMEM), or when vfs_tmpfile() later fails in may_open() or
hook_file_open() — the filesystem's error path calls discard_new_inode(),
iget_failed(), or iput():

fs/inode.c:discard_new_inode() {
	...
	inode_state_clear(inode, I_NEW);
	inode_wake_up_bit(inode, __I_NEW);
	spin_unlock(&inode->i_lock);
	iput(inode);
}

Because hierarchy->created still holds a reference to the inode, iput(inode)
only drops i_count from 2 to 1 instead of running iput_final() -> evict() ->
remove_inode_hash(). For filesystems that hash new inodes via
insert_inode_locked() before calling security_inode_init_security() (such as
ext2, jfs, gfs2, btrfs, or sysv), doesn't this leave an uninstantiated inode
with I_CREATING set in inode_hashtable until the domain exits, causing
subsequent allocations of the reused on-disk inode number to fail with -EBUSY
in insert_inode_locked() or -ESTALE in find_inode_fast()?

[Severity: High]
Is it safe for hook_inode_init_security() to allocate memory with
GFP_KERNEL_ACCOUNT via grant_created()?

Filesystems invoke security_inode_init_security() in the middle of inode
creation while holding internal filesystem transactions, mutexes, glocks, or
locked metadata folios without memalloc_nofs_save() (for example, jfs_create()
holds a transaction and commit_mutex, f2fs_init_inode_metadata() holds
f2fs_lock_op() and a locked inode folio, btrfs_create_new_inode() holds a
transaction handle, and gfs2_create_inode() holds glocks), which is why
security_inode_init_security() in security/security.c uses GFP_NOFS.

However, hook_inode_init_security() -> grant_created() allocates struct
landlock_object in get_inode_object() -> landlock_create_object() and struct
landlock_created in landlock_add_created() using GFP_KERNEL_ACCOUNT (which
includes __GFP_FS). Can direct reclaim recurse into the filesystem and
deadlock on those held locks or transactions?

[Severity: Medium]
Does relying on hook_inode_init_security() miss filesystems that do not call
security_inode_init_security(), and keep unlinked files' backing storage
allocated until the domain exits?

First, landlock_append_fs_rule() accepts LANDLOCK_ADD_RULE_CREATED on any
directory, but security_inode_init_security() is not called at the VFS layer:
in ext4, ext2, f2fs, jfs, jffs2, and ubifs it is only compiled in when
CONFIG_*_FS_SECURITY=y (where CONFIG_EXT4_FS_SECURITY and
CONFIG_F2FS_FS_SECURITY default to n), and in filesystems without security
xattr support (such as fuse/virtiofs, nfs, 9p, cifs, vfat, exfat, ntfs3, udf,
and nilfs2) it is never called at all. On those filesystems or kernel
configurations, creating a file with open(O_CREAT | O_RDWR) in a
LANDLOCK_ADD_RULE_CREATED directory will never trigger
hook_inode_init_security(), causing the file to be created on disk and then
rejected with -EACCES in hook_file_open().

Second, because landlock_add_created() pins each created inode via ihold() in
hierarchy->created until landlock_free_created() runs when the layer's
hierarchy is freed, iput_final() -> evict() cannot run when created files are
unlinked. If a long-lived sandbox or build daemon creates and unlinks temporary
files in a LANDLOCK_ADD_RULE_CREATED directory, won't their on-disk blocks and
tmpfs pages remain allocated until every task in that domain layer exits?

> @@ -1623,7 +1767,8 @@ static int hook_path_link(struct dentry *const old_dentry,
>  			  struct dentry *const new_dentry)
>  {
>  	return current_check_refer_path(old_dentry, new_dir, new_dentry, false,
> -					false, false);
> +					false, false) ?:
> +		       refer_created(old_dentry, new_dir);
>  }
>  
>  static int hook_path_rename(const struct path *const old_dir,
> @@ -1632,10 +1777,16 @@ static int hook_path_rename(const struct path *const old_dir,
>  			    struct dentry *const new_dentry,
>  			    const unsigned int flags)
>  {
> +	int err;
> +
>  	/* old_dir refers to old_dentry->d_parent and new_dir->mnt */
> -	return current_check_refer_path(old_dentry, new_dir, new_dentry, true,
> -					!!(flags & RENAME_EXCHANGE),
> -					!!(flags & RENAME_WHITEOUT));
> +	err = current_check_refer_path(old_dentry, new_dir, new_dentry, true,
> +				       !!(flags & RENAME_EXCHANGE),
> +				       !!(flags & RENAME_WHITEOUT));
> +	if (err || (flags & RENAME_EXCHANGE))
> +		return err;
> +
> +	return refer_created(old_dentry, new_dir);
>  }

[Severity: High]
Doesn't calling refer_created() in hook_path_link() and hook_path_rename()
permanently grant access rights and pin the inode in hierarchy->created before
the VFS validates or performs the link or rename?

In fs/namei.c, security_path_link() and security_path_rename() run before
vfs_link() and vfs_rename():

fs/namei.c:filename_renameat2() {
	...
	error = security_path_rename(&old_path, rd.old_dentry,
				     &new_path, rd.new_dentry, flags);
	if (error)
		goto exit_unlock;

	error = vfs_rename(&rd);
	...
}

At the point hook_path_link() and hook_path_rename() run, the VFS has not yet
checked DAC permissions (may_create(), may_delete_dentry(),
inode_permission()), file-type constraints (such as S_ISDIR() returning -EPERM
in vfs_link(), or -EISDIR / -ENOTDIR / -ENOTEMPTY in vfs_rename()),
subsequent LSM hooks (security_inode_link(), security_inode_rename()), or the
filesystem's ->link() and ->rename() operations.

If vfs_link() or vfs_rename() fails, refer_created() has already inserted
old_dentry's inode into hierarchy->created and pinned it via ihold(). Could a
sandboxed task issue a link() or rename() that is guaranteed to fail in the
VFS (for example, calling link() on a directory, which fails with -EPERM in
vfs_link(), or renaming onto an incompatible target or DAC-unwritable
directory) to permanently stamp new_dir's LANDLOCK_ADD_RULE_CREATED rights
onto an inode in another directory, and then move that inode into a more
restricted directory while retaining those rights?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-landlock-created-rights-v1-0-77c8db3f6bd2@gmail.com?part=2