On Wed, Sep 22, 2021 at 10:57 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> I wanted to check in on this PR to see if you were planning on merging
> it for v5.15-rcX, kicking it back for -next instead, or simply glaring
> at it with quiet disgust?
Heh. I glanced at it with quiet disgust when it came in, and then it
just got lost.
But now I'm looking at it again since you reminded me, and I don't
understand why it has to be done in such an ugly manner.
And I also don't think the thing makes a whit of sense in the first place.
Honestly, that whole "lockdown depends on current creds" makes NO
SENSE at all to me. The whole and only point about lockdown is that
it's locked down. Not that "oh, root can do X". That's against the
point.
The whole and only point of lockdown was as a global thing. Now
somebody seems to have noticed that they violated that basic rule, and
that it's about permissions after all, and it's just a complete mess.
But why the heck would a normal lockdown user have to care about this
fundamental design mistake? Why should a normal lockdown user pass in
a credential that doesn't make sense?
Since 99% of all users DON'T want special rules, why isn't the normal
security_locked_down() kept the way it is?
Make the few special cases do special things, in other words.
This is *literally* why we have all those wrapper functions in
security/security.c - so that people can do sane interfaces and not
call down to the raw hooks.
(Yeah, yeah, a lot of them do nothing but pass it down, but others do
other sanity stuff so that callers don't have to do pointless
boilerplate)
IOW, why is this changing all the normal users that *really* don't
want it, and that really have absolutely no business looking up the
current creds?
Make the regular security_locked_down() function do that, and add a
if (WARN_ON_ONCE(!in_task()))
return -EPERM;
so that any bad cases get flagged and refuse to continue.
And then the couple of special users (whether due to interrupt context
or whatever), could get their own wrapper function.
Note how that would
(a) make the patch smaller
(b) not pollute normal users pointlessly
(c) actually be a kind of documentation too
(d) not make the default lockdown testing function be senseless
because right now you have absolutely no explanation in the crazy
cases, so y9ou have code like this:
lockdown = !!security_locked_down(NULL, LOCKDOWN_XMON_RW);
in xmon_is_locked_down(), and it makes no sense to anybody. Why the
NULL? What is going on?
Yes, yes, I can tell why the NULL, and what is going on, because I
read the commit message and it's in my context. But look at that line,
and tell me that it makes sense as code to anybody who has paged out
that context (or never had it in the first place).
If it said "security_globally_locked_down()" maybe it would at least
give a hint about what's going on.
But the other side of the argument is that the *common* lockdown
functions are insane too after the patch. In kernel/module.c, this
line:
return security_locked_down(current_cred(), LOCKDOWN_MODULE_SIGNATURE);
really screams "lockdown is fundamentally broken and mis-designed".
Seriously. If lockdown needs "current creds" then lockdown is wrong.
I was unhappy about lockdown from before, so maybe I'm more likely to
just reject this kind of garbage, but this really looks completely
broken to me.
Hmm? This lockdown stuff is some ugly random code to begin with, I
think the interfaces need to make SENSE. Passing in credentials
fundamentally does not make sense to me.
Linus
