[GIT PULL] capabilities

[Jordan Rome]

On Wed, Nov 27, 2024 at 4:42 PM Serge E. Hallyn <serge at hallyn.com> wrote:
>
> On Wed, Nov 27, 2024 at 09:30:14AM -0800, Linus Torvalds wrote:
> > On Mon, 18 Nov 2024 at 07:26, <sergeh at kernel.org> wrote:
> > >
> > > 2. Add a trace event for cap_capable (Jordan Rome).
> >
> > So I've finally gotten around to this, but I absolutely detest how
> > this was written.
> >
> > It was oddly written before, but now it's absolutely illegible.  All
> > just to have one single tracepoint.
> >
> > And it's all *stupid*.
> >
> > The "capable_ns" thing is entirely pointless.
> >
> > Why? It always has exactly one value: 'cred->user_ns'. Lookie here,
> > it's assigned exactly twice:
> >
> >                 if (ns == cred->user_ns) {
> >                         if (cap_raised(cred->cap_effective, cap)) {
> >                                 capable_ns = ns;
> > ...
> >                 if ((ns->parent == cred->user_ns) && uid_eq(ns->owner,
> > cred->euid)) {
> >                         capable_ns = ns->parent;
> >
> > and *both* times it's assigned something that we just checked is equal
> > to cred->user_ns.
> >
> > And for this useless value, the already odd for-loop was written to be
> > even more odd, and the code added a new variable 'capable_ns'.
> >
> > So I pulled this, tried to figure out _why_ it was written that oddly,
> > decided that the "why" was "because it's being stupid", and I unpulled
> > it again.
> >
> > If we really need that trace point, I have a few requirements:
> >
> >  - none of this crazy stuff
> >
> >  - use a simple inline helper
> >
> >  - make the pointers 'const', because there is no reason not to.
> >
> > Something *UNTESTED* like the attached diff.
> >
> > Again: very untested. But at least this generates good code, and
> > doesn't have pointless crazy variables. Yes, I add that
> >
> >         const struct user_namespace *cred_ns = cred->user_ns;
> >
> > because while I think gcc may be smart enough to figure out that it's
> > all the same value, I wanted to make sure.
> >
> > Then the tracepoint would look something like
> >
> >         trace_cap_capable(cred, targ_ns,  cred_ns, cap, opts, ret);
> >
> > although I don't understand why you'd even trace that 'opts' value
> > that is never used.
>
> You mean cap_capable doesn't use opts?  Yeah, it's used only by other
> LSMs.  I suppose knowing the value might in some cases help to figure
> out caller state, but dropping it seems sensible.
>
> Jordan is working on a new version based on your feedback.
>
> thanks,
> -serge

Here is the new patch:
https://patchwork.kernel.org/project/linux-security-module/patch/20241128153733.1542817-1-linux@jordanrome.com/

I applied the suggested changes but left the local `struct
user_namespace *ns` in the helper (as it gets updated in the loop to
the ns parent).
Though it feels a bit icky that there is not a null check against the
`ns` variable (maybe it's not possible to reach that condition
though).


Jordan