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@xxxxxxxxxx> > Cc: Paul Moore <paul@xxxxxxxxxxxxxx> > Cc: Serge Hallyn <serge@xxxxxxxxxx> > Cc: Tahera Fahimi <fahimitahera@xxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx > Acked-by: Christian Brauner <brauner@xxxxxxxxxx> > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx> > Link: https://lore.kernel.org/r/20250318161443.279194-6-mic@xxxxxxxxxxx > 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; >
