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
