Message-Id: <D36G6JFV79FK.2LCFXJYDTRRPK@kernel.org>
Date: Sat, 03 Aug 2024 20:20:35 +0300
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Jann Horn" <jannh@...gle.com>, "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>,
 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 9:39 PM EEST, Jann Horn wrote:
> > > 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?

Or ERESTARTSYS if you want to select the behavior from caller using
SA_RESTART, whether to restart or -EINTR.


> > > 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)

Yeah, I'd go for this simply because it allows better reviewer
visibility. You can look at the soluton and cleanups separately.

>
> [...]
> > 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.)

I don't want to dictate this because might give wrong advice (given
bandwidth to think it through). Pick a solution and we'll look at it :-)

BR, Jarkko

Powered by blists - more mailing lists