Re: [PATCH] landlock: Fix domain leak on concurrent F_SETOWN and file release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] landlock: signal restriction bypass via concurrent FASYNC SIGIO and fput
- [Medium] landlock: dead code introduced to fix impossible F_SETOWN vs fput race
--

commit 051033b270aa84fe01c7dc8386b6ca0bc9d32596
Author: Jiakai Xu <xujiakai24@mails.ucas.ac.cn>

landlock: Fix domain leak on concurrent F_SETOWN and file release

This commit adds a file_release hook to synchronously clear Landlock domain
references during file release. It attempts to prevent a domain leak caused
by a concurrent F_SETOWN operation and the final fput().

> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index f7e5e4ef9eac3..8261aa55b0127 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
[ ... ]
> @@ -1971,8 +1971,49 @@ static void hook_file_set_fowner(struct file *file)
>  	put_pid(prev_tg);
>  }
>  
> +/*
> + * Drops the Landlock references saved by hook_file_set_fowner(), in a
> + * critical section serialized with it thanks to file->f_owner->lock, and
> + * before file_f_owner_release() frees this lock.  Without this mutual
> + * exclusion, a concurrent F_SETOWN could store a new domain reference into a
> + * file being released (the last fput() made it unreachable to future F_SETOWN
> + * users), which would then never be dropped, leaking the whole domain.
> + */
> +static void hook_file_release(struct file *file)
> +{

[Severity: Medium]
Is it possible for sys_fcntl(F_SETOWN) and the final __fput() to actually
run concurrently?

sys_fcntl() uses fdget(), which acquires a reference on the struct file,
ensuring f_count > 0. __fput() is only executed when f_count drops to 0. It
appears these paths cannot execute concurrently.

If they could execute concurrently, wouldn't __fput() freeing f_owner
while F_SETOWN is locking it cause a massive VFS Use-After-Free,
independent of Landlock? This hook might be dead code addressing an
impossible race condition.

> +	struct landlock_ruleset *prev_dom;
> +	struct pid *prev_tg;
> +	struct fown_struct *fown;
> +
> +	fown = file_f_owner(file);
> +	if (!fown)
> +		/* No owner was ever recorded, cf. hook_file_set_fowner(). */
> +		return;
> +
> +	/*
> +	 * __fput() calls this hook before file_f_owner_release(), so the
> +	 * fown_struct is still alive here.
> +	 */
> +	write_lock_irq(&fown->lock);
> +	prev_dom = landlock_file(file)->fown_subject.domain;
> +	prev_tg = landlock_file(file)->fown_tg;
> +	landlock_file(file)->fown_subject.domain = NULL;
> +	landlock_file(file)->fown_tg = NULL;

[Severity: Critical]
Could clearing the domain here introduce a signal restriction bypass?

In __fput(), security_file_release() is called before the file is safely
removed from the asynchronous signal delivery mechanism:

fs/file_table.c:__fput() {
    ...
	security_file_release(file);
	if (unlikely(file->f_flags & FASYNC)) {
		if (file->f_op->fasync)
			file->f_op->fasync(-1, file, 0);
	}
    ...
}

Because hook_file_release() clears fown_subject.domain here, the file
remains on the device's fasync list without its domain restrictions until
file->f_op->fasync() is called.

If a hardware interrupt (such as a network packet or TTY input) occurs in
this window, kill_fasync() will send a SIGIO. hook_file_send_sigiotask()
will see subject->domain == NULL and incorrectly allow the restricted
signal to be delivered to a sandboxed process.

> +	write_unlock_irq(&fown->lock);
> +
> +	/* May be called in an RCU read-side critical section. */
> +	landlock_put_ruleset_deferred(prev_dom);
> +	put_pid(prev_tg);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924100206.173523-1-xujiakai24@mails.ucas.ac.cn?part=1