[GIT PULL] capabilities

[GIT PULL] capabilities

[sergeh]

Hi Linus,

The following changes since commit 9852d85ec9d492ebef56dc5f229416c925758edc:

  Linux 6.12-rc1 (2024-09-29 15:06:19 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/sergeh/linux.git tags/caps-6.12-rc1

for you to fetch changes up to 54eb2cec2ed7aaf25524dd5ebeaa4f87ed5c6bd6:

  security: add trace event for cap_capable (2024-10-30 14:40:02 -0500)

This branch contains two patches:

1. Remove the cap_mmap_file() hook (Paul Moore), as it wasn't actually doing anything.
   de2433c608c2c2633b8a452dd4925d876f3d5add

2. Add a trace event for cap_capable (Jordan Rome).
   54eb2cec2ed7aaf25524dd5ebeaa4f87ed5c6bd6

Both patches have been sitting in linux-next for quite some time with no apparent
issues.

thanks,
-serge

----------------------------------------------------------------
Jordan Rome (1):
      security: add trace event for cap_capable

Paul Moore (1):
      capabilities: remove cap_mmap_file()

 MAINTAINERS                       |  1 +
 include/trace/events/capability.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 security/commoncap.c              | 37 ++++++++++++++++++++++---------------
 3 files changed, 83 insertions(+), 15 deletions(-)
 create mode 100644 include/trace/events/capability.h

Re: [GIT PULL] capabilities

[Serge E. Hallyn]

On Mon, Nov 18, 2024 at 03:26:31PM +0000, sergeh@kernel.org wrote:
> Hi Linus,

Hi,

before the merge window closes, I just wanted to make sure this didn't get
lost.

thanks,
-serge

> The following changes since commit 9852d85ec9d492ebef56dc5f229416c925758edc:
> 
>   Linux 6.12-rc1 (2024-09-29 15:06:19 -0700)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/sergeh/linux.git tags/caps-6.12-rc1
> 
> for you to fetch changes up to 54eb2cec2ed7aaf25524dd5ebeaa4f87ed5c6bd6:
> 
>   security: add trace event for cap_capable (2024-10-30 14:40:02 -0500)
> 
> This branch contains two patches:
> 
> 1. Remove the cap_mmap_file() hook (Paul Moore), as it wasn't actually doing anything.
>    de2433c608c2c2633b8a452dd4925d876f3d5add
> 
> 2. Add a trace event for cap_capable (Jordan Rome).
>    54eb2cec2ed7aaf25524dd5ebeaa4f87ed5c6bd6
> 
> Both patches have been sitting in linux-next for quite some time with no apparent
> issues.
> 
> thanks,
> -serge
> 
> ----------------------------------------------------------------
> Jordan Rome (1):
>       security: add trace event for cap_capable
> 
> Paul Moore (1):
>       capabilities: remove cap_mmap_file()
> 
>  MAINTAINERS                       |  1 +
>  include/trace/events/capability.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  security/commoncap.c              | 37 ++++++++++++++++++++++---------------
>  3 files changed, 83 insertions(+), 15 deletions(-)
>  create mode 100644 include/trace/events/capability.h

Re: [GIT PULL] capabilities

[Paul Moore]

On Fri, Nov 22, 2024 at 12:34 PM Serge E. Hallyn <serge@hallyn.com> wrote:
>
> On Mon, Nov 18, 2024 at 03:26:31PM +0000, sergeh@kernel.org wrote:
> > Hi Linus,
>
> Hi,
>
> before the merge window closes, I just wanted to make sure this didn't get
> lost.

... and while Serge may not have sent a pull request for the
capability code in a while, I just want to vouch that Serge is a real
person and this is a legit pull request :)

-- 
paul-moore.com

Re: [GIT PULL] capabilities

[Serge E. Hallyn]

On Mon, Nov 25, 2024 at 01:52:59PM -0500, Paul Moore wrote:
> On Fri, Nov 22, 2024 at 12:34 PM Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > On Mon, Nov 18, 2024 at 03:26:31PM +0000, sergeh@kernel.org wrote:
> > > Hi Linus,
> >
> > Hi,
> >
> > before the merge window closes, I just wanted to make sure this didn't get
> > lost.
> 
> ... and while Serge may not have sent a pull request for the
> capability code in a while, I just want to vouch that Serge is a real
> person and this is a legit pull request :)

Hi Linus,

don't mean to nag, but as the end of the merge window is approaching,
figure I should try one more time (or else ask Paul to send it instead).
Did you receive this PR?  Are there any changes I need to make?

thanks,
-serge

Re: [GIT PULL] capabilities

[Linus Torvalds]

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.

            Linus

Re: [GIT PULL] capabilities

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

On Wed, 27 Nov 2024 at 09:30, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Something *UNTESTED* like the attached diff.

Well, that "attached diff" wass again something I forgot to actually attach.

Here.

Again: very much untested. But at least the code makes a bit of sense,
and code generation is actually improved by this.

                Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1991 bytes --]

 security/commoncap.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index cefad323a0b1..2fadbda75131 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -64,18 +64,18 @@ static void warn_setuid_and_fcaps_mixed(const char *fname)
  * cap_has_capability() returns 0 when a task has a capability, but the
  * kernel's capable() and has_capability() returns 1 for this case.
  */
-int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
-		int cap, unsigned int opts)
+static int cap_capable_helper(const struct cred *cred,
+				const struct user_namespace *ns,
+				const struct user_namespace *cred_ns,
+				int cap)
 {
-	struct user_namespace *ns = targ_ns;
-
 	/* See if cred has the capability in the target user namespace
 	 * by examining the target user namespace and all of the target
 	 * user namespace's parents.
 	 */
 	for (;;) {
 		/* Do we have the necessary capabilities? */
-		if (ns == cred->user_ns)
+		if (likely(ns == cred_ns))
 			return cap_raised(cred->cap_effective, cap) ? 0 : -EPERM;
 
 		/*
@@ -89,7 +89,7 @@ int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
 		 * The owner of the user namespace in the parent of the
 		 * user namespace has all caps.
 		 */
-		if ((ns->parent == cred->user_ns) && uid_eq(ns->owner, cred->euid))
+		if ((ns->parent == cred_ns) && uid_eq(ns->owner, cred->euid))
 			return 0;
 
 		/*
@@ -102,6 +102,15 @@ int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
 	/* We never get here */
 }
 
+int cap_capable(const struct cred *cred, struct user_namespace *target_ns,
+	int cap, unsigned int opts)
+{
+	const struct user_namespace *cred_ns = cred->user_ns;
+	int ret = cap_capable_helper(cred, target_ns, cred_ns, cap);
+	/* tracepoint here */
+	return ret;
+}
+
 /**
  * cap_settime - Determine whether the current process may set the system clock
  * @ts: The time to set

Re: [GIT PULL] capabilities

[Linus Torvalds]

On Wed, 27 Nov 2024 at 09:31, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Again: very much untested. But at least the code makes a bit of sense,
> and code generation is actually improved by this.

Oh, and the 'cap_capable_helper()' function should be marked inline.
Gcc does do it automatically for this kind of static functions only
used once, but just for clarity it's best to mark it explicitly.

            Linus

Re: [GIT PULL] capabilities

[Serge E. Hallyn]

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

Re: [GIT PULL] capabilities

[Jordan Rome]

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).


Jordan

Re: [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@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...

Re: [GIT PULL] capabilities

[Serge E. Hallyn]

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.