[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z2NIpSZ9iY0q1EAl@gallifrey>
Date: Wed, 18 Dec 2024 22:11:49 +0000
From: "Dr. David Alan Gilbert" <linux@...blig.org>
To: Paul Moore <paul@...l-moore.com>
Cc: serge@...lyn.com, linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] capability: Remove unused has_capability
* Paul Moore (paul@...l-moore.com) wrote:
> On Sun, Dec 15, 2024 at 11:54 AM <linux@...blig.org> wrote:
> >
> > From: "Dr. David Alan Gilbert" <linux@...blig.org>
> >
> > The vanilla has_capability() function has been unused since 2018's
> > commit dcb569cf6ac9 ("Smack: ptrace capability use fixes")
> >
> > Remove it.
> >
> > (There is still mention in a comment in security/commoncap.c
> > but I suspect rather than removing the entry it might be better
> > to expand the comment to talk about the other
> > has_[ns_]capability[_noaudit] variants).
Hi Paul,
Thanks for the review,
> I would suggest that this patch would be an excellent place to change
> that comment. Without historical knowledge, the comment will be hard
> to understand after this patch is merged as inspecting
> has_capability() will be much more difficult, and including the
> comment change with the function removal will bind the two changes
> nicely in the git log.
Yeh, how would you like it? The existing comment is:
'
* NOTE WELL: cap_has_capability() cannot be used like the kernel's capable()
* and has_capability() functions. That is, it has the reverse semantics:
* cap_has_capability() returns 0 when a task has a capability, but the
* kernel's capable() and has_capability() returns 1 for this case.
'
For a start I think that's wrong; the function it's above is
'cap_capable()' not 'cap_has_capability()' - and has been for 15 years :-)
How about:
'
* NOTE WELL: cap_capable() has reverse semantics to the other kernel
* functions. That is cap_capable() returns 0 when a task has a capability,
* the kernel's capable(), has_ns_capability(), has_ns_capability_noaudit(),
* and has_capability_noaudit() return 1 for this case.
'
(It's curious how rarely most of these are used...)
> Otherwise, this seems fine to me.
>
> Reviewed-by: Paul Moore <paul@...l-moore.com>
Thanks,
Dave
> > Signed-off-by: Dr. David Alan Gilbert <linux@...blig.org>
> > ---
> > include/linux/capability.h | 5 -----
> > kernel/capability.c | 16 ----------------
> > 2 files changed, 21 deletions(-)
> >
> > diff --git a/include/linux/capability.h b/include/linux/capability.h
> > index 0c356a517991..1fb08922552c 100644
> > --- a/include/linux/capability.h
> > +++ b/include/linux/capability.h
> > @@ -139,7 +139,6 @@ static inline kernel_cap_t cap_raise_nfsd_set(const kernel_cap_t a,
> > }
> >
> > #ifdef CONFIG_MULTIUSER
> > -extern bool has_capability(struct task_struct *t, int cap);
> > extern bool has_ns_capability(struct task_struct *t,
> > struct user_namespace *ns, int cap);
> > extern bool has_capability_noaudit(struct task_struct *t, int cap);
> > @@ -150,10 +149,6 @@ extern bool ns_capable(struct user_namespace *ns, int cap);
> > extern bool ns_capable_noaudit(struct user_namespace *ns, int cap);
> > extern bool ns_capable_setid(struct user_namespace *ns, int cap);
> > #else
> > -static inline bool has_capability(struct task_struct *t, int cap)
> > -{
> > - return true;
> > -}
> > static inline bool has_ns_capability(struct task_struct *t,
> > struct user_namespace *ns, int cap)
> > {
> > diff --git a/kernel/capability.c b/kernel/capability.c
> > index dac4df77e376..67094b628ea9 100644
> > --- a/kernel/capability.c
> > +++ b/kernel/capability.c
> > @@ -289,22 +289,6 @@ bool has_ns_capability(struct task_struct *t,
> > return (ret == 0);
> > }
> >
> > -/**
> > - * has_capability - Does a task have a capability in init_user_ns
> > - * @t: The task in question
> > - * @cap: The capability to be tested for
> > - *
> > - * Return true if the specified task has the given superior capability
> > - * currently in effect to the initial user namespace, false if not.
> > - *
> > - * Note that this does not set PF_SUPERPRIV on the task.
> > - */
> > -bool has_capability(struct task_struct *t, int cap)
> > -{
> > - return has_ns_capability(t, &init_user_ns, cap);
> > -}
> > -EXPORT_SYMBOL(has_capability);
> > -
> > /**
> > * has_ns_capability_noaudit - Does a task have a capability (unaudited)
> > * in a specific user ns.
> > --
> > 2.47.1
>
> --
> paul-moore.com
>
--
-----Open up your eyes, open up your mind, open up your code -------
/ Dr. David Alan Gilbert | Running GNU/Linux | Happy \
\ dave @ treblig.org | | In Hex /
\ _________________________|_____ http://www.treblig.org |_______/
Powered by blists - more mailing lists