linux-kernel - Re: [PATCH] capability: Remove unused has

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhTmqMKkemeyWK3d6tyPGSus9ApMpZzTjtrmgHqbC_au+Q@mail.gmail.com>
Date: Wed, 18 Dec 2024 21:24:58 -0500
From: Paul Moore <paul@...l-moore.com>
To: "Dr. David Alan Gilbert" <linux@...blig.org>
Cc: serge@...lyn.com, linux-security-module@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] capability: Remove unused has_capability

On Wed, Dec 18, 2024 at 5:11 PM Dr. David Alan Gilbert
<linux@...blig.org> wrote:
> * 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 :-)

The code in security/commoncap.c is fairly mature and stable, and I
don't expect that many people spend a lot of time in that file, I know
I don't.  An unfortunate side effect is that certain things that
aren't caught by a compiler can easily go out of date, and stay that
way for some time :/

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

Two things come to mind when reading the suggested comment:

* I don't like the "... reverse semantics to the other kernel
functions" text simply because the majority of kernel functions do
follow the "0 on success, negative errno on failure" pattern that we
see in cap_capable().  I would suggest something along the lines of
"... reverse semantics of the capable() call".

* Most (all?) of the capable() family of functions, excluding
cap_capable() of course, return a bool value, true/false, instead of
non-zero/zero.  If we're going to complain about the existing comment,
we probably should get this correct ;)

-- 
paul-moore.com