Re: [GIT PULL] capabilities

["Serge E. Hallyn" <serge@hallyn.com>]

On Thu, Nov 28, 2024 at 10:28:56AM -0600, Serge E. Hallyn wrote:
> 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@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@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...

Sorry, I don't know how I missed the use of cred->cap_effective.  Please ignore.