[GIT PULL] capabilities

[Serge E. Hallyn]

On Thu, Nov 28, 2024 at 10:42:16AM -0500, Jordan Rome wrote:
> 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).

Feels like a lot of bikeshedding here, but...

Is there any reason at this point not to just pass in cred_euid as a uid_t,
and not pass the cred in at all?  Avoid a few more dereferences...