Re: [PATCH RFC] security/KEYS: get rid of cred_alloc_blank and cred_transfer

["Jarkko Sakkinen" <jarkko@kernel.org>]

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