Message-ID: <CAG48ez1GFY5H1ujaDfcj-Ay5_Pm8MsBVL=vU4tEynXgzg5yduQ@mail.gmail.com>
Date: Fri, 2 Aug 2024 20:39:56 +0200
From: Jann Horn <jannh@...gle.com>
To: Jarkko Sakkinen <jarkko.sakkinen@....fi>
Cc: Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, 
	"Serge E. Hallyn" <serge@...lyn.com>, John Johansen <john.johansen@...onical.com>, 
	David Howells <dhowells@...hat.com>, Jarkko Sakkinen <jarkko@...nel.org>, 
	Mickaël Salaün <mic@...ikod.net>, 
	Günther Noack <gnoack@...gle.com>, 
	Stephen Smalley <stephen.smalley.work@...il.com>, Ondrej Mosnacek <omosnace@...hat.com>, 
	Casey Schaufler <casey@...aufler-ca.com>, linux-kernel@...r.kernel.org, 
	linux-security-module@...r.kernel.org, apparmor@...ts.ubuntu.com, 
	keyrings@...r.kernel.org, selinux@...r.kernel.org
Subject: Re: [PATCH RFC] security/KEYS: get rid of cred_alloc_blank and cred_transfer

On Fri, Aug 2, 2024 at 8:09 PM Jarkko Sakkinen <jarkko.sakkinen@....fi> wrote:
> On Fri Aug 2, 2024 at 4:10 PM EEST, Jann Horn wrote:
> > cred_alloc_blank and cred_transfer were only necessary so that keyctl can
> > allocate creds in the child and then asynchronously have the parent fill
> > them in and apply them.
> >
> > Get rid of them by letting the child synchronously wait for the task work
> > executing in the parent's context. This way, any errors that happen in the
> > task work can be plumbed back into the syscall return value in the child.
> >
> > Note that this requires using TWA_SIGNAL instead of TWA_RESUME, so the
> > parent might observe some spurious -EGAIN syscall returns or such; but the
> > parent likely anyway has to be ready to deal with the side effects of
> > receiving signals (since it'll probably get SIGCHLD when the child dies),
> > so that probably isn't an issue.
> >
> > Signed-off-by: Jann Horn <jannh@...gle.com>
> > ---
> > This is a quickly hacked up demo of the approach I proposed at
> > <https://lore.kernel.org/all/CAG48ez2bnvuX8i-D=5DxmfzEOKTWAf-DkgQq6aNC4WzSGoEGHg@mail.gmail.com/>
> > to get rid of the cred_transfer stuff. Diffstat looks like this:
> >
> >  include/linux/cred.h          |   1 -
> >  include/linux/lsm_hook_defs.h |   3 ---
> >  include/linux/security.h      |  12 ------------
> >  kernel/cred.c                 |  23 -----------------------
> >  security/apparmor/lsm.c       |  19 -------------------
> >  security/keys/internal.h      |   8 ++++++++
> >  security/keys/keyctl.c        | 100 ++++++++++++++++++++++++++--------------------------------------------------------------------------
> >  security/keys/process_keys.c  |  86 ++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------
> >  security/landlock/cred.c      |  11 ++---------
> >  security/security.c           |  35 -----------------------------------
> >  security/selinux/hooks.c      |  12 ------------
> >  security/smack/smack_lsm.c    |  32 --------------------------------
> >  12 files changed, 82 insertions(+), 260 deletions(-)
> >
> > What do you think? Synchronously waiting for task work is a bit ugly,
> > but at least this condenses the uglyness in the keys subsystem instead
> > of making the rest of the security subsystem deal with this stuff.
>
> Why does synchronously waiting is ugly? Not sarcasm, I genuineily
> interested of breaking that down in smaller pieces.
>
> E.g. what disadvantages would be there from your point of view?
>
> Only trying to form a common picture, that's all.

Two things:

1. It means we have to send a pseudo-signal to the parent, to get the
parent to bail out into signal handling context, which can lead to
extra spurious -EGAIN in the parent. I think this is probably fine
since _most_ parent processes will already expect to handle SIGCHLD
signals...

2. If the parent is blocked on some other killable wait, we won't be
able to make progress - so in particular, if the parent was using a
killable wait to wait for the child to leave its syscall, userspace
ẁould deadlock (in a way that could be resolved by SIGKILLing one of
the processes). Actually, I think that might happen if the parent uses
ptrace() with sufficiently bad timing? We could avoid the issue by
doing an interruptible wait instead of a killable one, but then that
might confuse userspace callers of the keyctl() if they get an
-EINTR...
I guess the way to do this cleanly is to use an interruptible wait and
return -ERESTARTNOINTR if it gets interrupted?

> > Another approach to simplify things further would be to try to move
> > the session keyring out of the creds entirely and just let the child
> > update it directly with appropriate locking, but I don't know enough
> > about the keys subsystem to know if that would maybe break stuff
> > that relies on override_creds() also overriding the keyrings, or
> > something like that.
> > ---
> >  include/linux/cred.h          |   1 -
> >  include/linux/lsm_hook_defs.h |   3 --
> >  include/linux/security.h      |  12 -----
> >  kernel/cred.c                 |  23 ----------
> >  security/apparmor/lsm.c       |  19 --------
> >  security/keys/internal.h      |   8 ++++
> >  security/keys/keyctl.c        | 100 +++++++++++-------------------------------
> >  security/keys/process_keys.c  |  86 +++++++++++++++++++-----------------
> >  security/landlock/cred.c      |  11 +----
> >  security/security.c           |  35 ---------------
> >  security/selinux/hooks.c      |  12 -----
> >  security/smack/smack_lsm.c    |  32 --------------
> >  12 files changed, 82 insertions(+), 260 deletions(-)
>
> Given the large patch size:
>
> 1. If it is impossible to split some meaningful patches, i.e. patches
>    that transform kernel tree from working state to another, I can
>    cope with this.
> 2. Even for small chunks that can be split into their own logical
>    pieces: please do that. Helps to review the main gist later on.

There are basically two parts to this, it could be split up nicely into these:

1. refactor code in security/keys/
2. rip out all the code that is now unused (as you can see in the
diffstat, basically everything outside security/keys/ is purely
removals)

[...]
> Not going through everything but can we e.g. make a separe SMACK patch
> prepending?

I wouldn't want to split it up further: As long as the cred_transfer
mechanism and LSM hook still exist, all the LSMs that currently have
implementations of it should also still implement it.

But I think if patch 2/2 is just ripping out unused infrastructure
across the tree, that should be sufficiently reviewable? (Or we could
split it up into ripping out one individual helper per patch, but IDK,
that doesn't seem to me like it adds much reviewability.)

Powered by blists - more mailing lists