Re: [PATCH v2 5/8] landlock: Always allow signals between threads of the same process

["Mickaël Salaün" <mic@digikod.net>]

On Tue, Mar 18, 2025 at 05:14:40PM +0100, Mickaël Salaün wrote:
> Because Linux credentials are managed per thread, user space relies on
> some hack to synchronize credential update across threads from the same
> process.  This is required by the Native POSIX Threads Library and
> implemented by set*id(2) wrappers and libcap(3) to use tgkill(2) to
> synchronize threads.  See nptl(7) and libpsx(3).  Furthermore, some
> runtimes like Go do not enable developers to have control over threads
> [1].
> 
> To avoid potential issues, and because threads are not security
> boundaries, let's relax the Landlock (optional) signal scoping to always
> allow signals sent between threads of the same process.  This exception
> is similar to the __ptrace_may_access() one.
> 
> hook_file_set_fowner() now checks if the target task is part of the same
> process as the caller.  If this is the case, then the related signal
> triggered by the socket will always be allowed.
> 
> Scoping of abstract UNIX sockets is not changed because kernel objects
> (e.g. sockets) should be tied to their creator's domain at creation
> time.
> 
> Note that creating one Landlock domain per thread puts each of these
> threads (and their future children) in their own scope, which is
> probably not what users expect, especially in Go where we do not control
> threads.  However, being able to drop permissions on all threads should
> not be restricted by signal scoping.  We are working on a way to make it
> possible to atomically restrict all threads of a process with the same
> domain [2].
> 
> Add erratum for signal scoping.
> 
> Closes: https://github.com/landlock-lsm/go-landlock/issues/36
> Fixes: 54a6e6bbf3be ("landlock: Add signal scoping")
> Fixes: c8994965013e ("selftests/landlock: Test signal scoping for threads")
> Depends-on: 26f204380a3c ("fs: Fix file_set_fowner LSM hook inconsistencies")
> Link: https://pkg.go.dev/kernel.org/pub/linux/libs/security/libcap/psx [1]
> Link: https://github.com/landlock-lsm/linux/issues/2 [2]
> Cc: Günther Noack <gnoack@google.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Serge Hallyn <serge@hallyn.com>
> Cc: Tahera Fahimi <fahimitahera@gmail.com>
> Cc: stable@vger.kernel.org
> Acked-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20250318161443.279194-6-mic@digikod.net

> index 71b9dc331aae..47c862fe14e4 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -27,7 +27,9 @@
>  #include <linux/mount.h>
>  #include <linux/namei.h>
>  #include <linux/path.h>
> +#include <linux/pid.h>
>  #include <linux/rcupdate.h>
> +#include <linux/sched/signal.h>
>  #include <linux/spinlock.h>
>  #include <linux/stat.h>
>  #include <linux/types.h>
> @@ -1630,15 +1632,27 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
>  
>  static void hook_file_set_fowner(struct file *file)
>  {
> -	struct landlock_ruleset *new_dom, *prev_dom;
> +	struct fown_struct *fown = file_f_owner(file);
> +	struct landlock_ruleset *new_dom = NULL;
> +	struct landlock_ruleset *prev_dom;
> +	struct task_struct *p;
>  
>  	/*
>  	 * Lock already held by __f_setown(), see commit 26f204380a3c ("fs: Fix
>  	 * file_set_fowner LSM hook inconsistencies").
>  	 */
> -	lockdep_assert_held(&file_f_owner(file)->lock);
> -	new_dom = landlock_get_current_domain();
> -	landlock_get_ruleset(new_dom);
> +	lockdep_assert_held(&fown->lock);
> +
> +	/*
> +	 * Always allow sending signals between threads of the same process.  This
> +	 * ensures consistency with hook_task_kill().
> +	 */
> +	p = pid_task(fown->pid, fown->pid_type);
> +	if (!same_thread_group(p, current)) {

There is a missing pointer check.  I'll apply this:

-       if (!same_thread_group(p, current)) {
+       if (!p || !same_thread_group(p, current)) {

> +		new_dom = landlock_get_current_domain();
> +		landlock_get_ruleset(new_dom);
> +	}
> +
>  	prev_dom = landlock_file(file)->fown_domain;
>  	landlock_file(file)->fown_domain = new_dom;
>