Message-ID: <20250311.aefai7vo6huW@digikod.net>
Date: Tue, 11 Mar 2025 15:32:53 +0100
From: Mickaël Salaün <mic@...ikod.net>
To: Günther Noack <gnoack3000@...il.com>
Cc: Paul Moore <paul@...l-moore.com>, sergeh@...nel.org, 
	David Howells <dhowells@...hat.com>, Kees Cook <keescook@...omium.org>, 
	linux-security-module@...r.kernel.org, Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, 
	Jann Horn <jannh@...gle.com>, linux-kernel@...r.kernel.org, 
	Peter Newman <peternewman@...gle.com>
Subject: Re: [RFC 1/2] landlock: Multithreading support for
 landlock_restrict_self()

On Mon, Mar 10, 2025 at 02:04:23PM +0100, Günther Noack wrote:
> Hello Paul and Serge!
> 
> On Tue, Mar 04, 2025 at 09:25:51PM +0100, Mickaël Salaün wrote:
> > On Fri, Feb 28, 2025 at 06:33:55PM +0100, Günther Noack wrote:
> > > Hello!
> > > 
> > > Thanks for the review!
> > > 
> > > I'm adding David Howells to this thread as well.  David, maybe you can
> > > help us and suggest a appropriate way to update the struct cred across
> > > multiple threads?
> 
> Paul and Serge, since you are volunteering to take ownership of
> credentials, maybe you can advise on what is the best approach here?
> 
> To summarize the approaches that I have been discussing with Mickaël:
> 
> Approach 1: Use the creds API thread-by-thread (implemented here)
> 
>   * Each task calls prepare_creds() and commit_creds() on its own, in
>     line with the way the API is designed to be used (from a single
>     task).
>   * Task work gets scheduled with a pseudo-signal and the task that
>     invoked the syscall is waiting for all of them to return.
>   * Task work can fail at the beginning due to prepare_creds(), in
>     which case all tasks have to abort_creds(). Additional
>     synchronization is needed for that.
> 
>   Drawback: We need to grab the system-global task lock to prevent new
>   thread creation and also grab the per-process signal lock to prevent
>   races with other creds accesses, for the entire time as we wait for
>   each task to do the task work.

In other words, this approach blocks all threads from the same process.

> 
> Approach 2: Attempt to do the prepare_creds() step in the calling task.
> 
>   * Would use an API similar to what keyctl uses for the
>     parent-process update.
>   * This side-steps the credentials update API as it is documented in
>     Documentation, using the cred_alloc_blank() helper and replicating
>     some prepare_creds() logic.
> 
>   Drawback: This would introduce another use of the cred_alloc_blank()
>   API (and the cred_transfer LSM hook), which would otherwise be
>   reasonable to delete if we can remove the keyctl use case.
>   (https://lore.kernel.org/all/20240805-remove-cred-transfer-v2-0-a2aa1d45e6b8@google.com/)

cred_alloc_blank() was designed to avoid dealing with -ENOMEM, which is
a required property for this Landlock TSYNC feature (i.e. atomic and
consistent synchronization).

I think it would make sense to replace most of the
key_change_session_keyring() code with a new cred_transfer() helper that
will memcpy the old cred to the new, increment the appropriate ref
counters, and call security_transfer_creds().  We could then use this
helper in Landlock too.

To properly handle race conditions with a thread changing its own
credentials, we would need a new LSM hook called by commit_creds().
For the Landlock implementation, this hook would check if the process is
being Landlocked+TSYNC and return -ERESTARTNOINTR if it is the case.
The newly created task_work would then be free to update each thread's
credentials while only blocking the calling thread (which is also a
required feature).

Alternatively, instead of a new LSM hook, commit_creds() could check
itself a new group leader's flag set if all the credentials from the
calling process are being updated, and return -ERESTARTNOINTR in this
case.

> 
> Approach 3: Store Landlock domains outside of credentials altogether
> 
>   * We could also store a task's Landlock domain as a pointer in the
>     per-task security blob, and refcount these.  We would need to make
>     sure that they get newly referenced and updated in the same
>     scenarios as they do within struct cred today.
>   * We could then guard accesses to a task's Landlock domain with a
>     more classic locking mechanism.  This would make it possible to
>     update the Landlock domain of all tasks in a process without
>     having to go through pseudo-signals.
> 
>   Drawbacks:
>   * Would have to make sure that the Landlock domain the task's LSM
>     blob behaves exactly the same as before in the struct cred.
>   * Potentially slower to access Landlock domains that are guarded by
>     a mutex.

This would not work because the kernel (including LSM hooks) uses
credentials to check access.

> 
> I'd be interested to hear your opinion on how we should best approach
> this.
> 
> P.S. This is probably already clear to everyone who read this far, but
> the problem that creds can't be updated across tasks has already lead
> to other difficult APIs and also bleeds into user-level interfaces
> such as the setuid() family of syscalls as well as capability
> updating.  Both of these are solved from user space through the signal
> hack documented in nptl(7), which is used in glibc for setuid()-style
> calls and in libpsx for capabilities and Landlock's Go library.  If we
> had a working way to do these cross-thread updates in the kernel, that
> would simplify these userspace implementations.  (There are more links
> in the cover letter at the top of this thread.)
> 
> Thanks,
> –Günther
> 

Powered by blists - more mailing lists