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

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

[Jiakai Xu]

The Landlock domain reference recorded by hook_file_set_fowner() was only
dropped by hook_file_free_security(), which runs from security_file_free()
without holding file->f_owner->lock.  hook_file_set_fowner() itself runs
under that lock (since commit 26f204380a3c ("fs: Fix file_set_fowner LSM
hook inconsistencies")), so the get-side and the put-side of the same
storage slot were not mutually exclusive.

If the final fput()/__fput() of a file runs concurrently with an
in-flight F_SETOWN on another CPU, the free path can drain the blob
(dropping the previously recorded domain) before the set path stores a
freshly acquired domain reference into it.  The new reference is then
orphaned: hook_file_free_security() has already run, so nothing will
ever drop it, and the whole domain (struct landlock_ruleset, its
landlock_hierarchy and its landlock_details) leaks.  kmemleak reports
this as a "memory leak in landlock_merge_ruleset" with the leaked
hierarchy showing usage=1 and parent=NULL.

Add a file_release hook, called by __fput() before
file_f_owner_release() (i.e. while the fown_struct and its lock are
still alive), which takes file->f_owner->lock, snapshots and clears the
recorded fown_subject/fown_tg references, and drops them outside the
lock.  This serializes the recorded reference lifecycle with
hook_file_set_fowner(), closing the store-after-put window in both
directions (the reverse interleaving would have been a double-put).
hook_file_free_security() stays as a no-op safety net for files without
a fown_struct.

Cc: stable@vger.kernel.org
Fixes: 54a6e6bbf3bef ("landlock: Add signal scoping")
Assisted-by: OpenCode:DeepSeek-V4-Flash
Signed-off-by: Jiakai Xu <xujiakai24@mails.ucas.ac.cn>
---
 security/landlock/fs.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

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)
+{
+	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;
+	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);
+}
+
 static void hook_file_free_security(struct file *file)
 {
+	/*
+	 * hook_file_release() already dropped and cleared these references if
+	 * they were ever recorded.  Keep a defensive cleanup for files without
+	 * a fown_struct (e.g. never owning files), which hook_file_release()
+	 * skips.
+	 */
 	put_pid(landlock_file(file)->fown_tg);
 	landlock_put_ruleset_deferred(landlock_file(file)->fown_subject.domain);
 }
@@ -2003,6 +2044,7 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
 	LSM_HOOK_INIT(file_ioctl_compat, hook_file_ioctl_compat),
 	LSM_HOOK_INIT(file_set_fowner, hook_file_set_fowner),
+	LSM_HOOK_INIT(file_release, hook_file_release),
 	LSM_HOOK_INIT(file_free_security, hook_file_free_security),
 };
 
-- 
2.34.1


Crash report (memory_leak_in_landlock_merge_ruleset_SyzGPT_20260917_162959_7d54763d, kernel 7.2-v3, appended below for reference; everything after the "-- " signature is discarded by git am):

---
BUG: memory leak
unreferenced object 0xffff88810fd82cc0 (size 96):
  comm "syz.1.2048", pid 20078, jiffies 4295170985
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 08 a4 0d 81 88 ff ff 00 00 00 00 00 00 00 00  ................
  backtrace (crc 5533466b):
    kmemleak_alloc_recursive home/zzzrrll/tmp/kf_src/linux-7.2.3/include/linux/kmemleak.h:44 [inline]
    slab_post_alloc_hook home/zzzrrll/tmp/kf_src/linux-7.2.3/mm/slub.c:4597 [inline]
    slab_alloc_node home/zzzrrll/tmp/kf_src/linux-7.2.3/mm/slub.c:4917 [inline]
    __do_kmalloc_node home/zzzrrll/tmp/kf_src/linux-7.2.3/mm/slub.c:5333 [inline]
    __kmalloc_noprof+0x210/0x4d0 home/zzzrrll/tmp/kf_src/linux-7.2.3/mm/slub.c:5359
    _kmalloc_noprof home/zzzrrll/tmp/kf_src/linux-7.2.3/include/linux/slab.h:992 [inline]
    _kzalloc_noprof home/zzzrrll/tmp/kf_src/linux-7.2.3/include/linux/slab.h:1309 [inline]
    create_ruleset home/zzzrrll/tmp/kf_src/linux-7.2.3/security/landlock/ruleset.c:36 [inline]
    landlock_merge_ruleset+0x8b/0x5d0 home/zzzrrll/tmp/kf_src/linux-7.2.3/security/landlock/ruleset.c:565
    __do_sys_landlock_restrict_self home/zzzrrll/tmp/kf_src/linux-7.2.3/security/landlock/syscalls.c:599 [inline]
    __se_sys_landlock_restrict_self+0x180/0x330 home/zzzrrll/tmp/kf_src/linux-7.2.3/security/landlock/syscalls.c:526
    do_syscall_x64 home/zzzrrll/tmp/kf_src/linux-7.2.3/arch/x86/entry/syscall_64.c:63 [inline]
    do_syscall_64+0x154/0x3b0 home/zzzrrll/tmp/kf_src/linux-7.2.3/arch/x86/entry/syscall_64.c:94
    entry_SYSCALL_64_after_hwframe+0x77/0x7f


<<<<<<<<<<<<<<< tail report >>>>>>>>>>>>>>>


---

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

[sashiko-bot]

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