[v3,1/2] capability: add cap_isidentical

  Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 include/linux/capability.h | 10 ++++++++++
 1 file changed, 10 insertions(+)
  

On Wed, Jan 25, 2023 at 7:56 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> +static inline bool cap_isidentical(const kernel_cap_t a, const kernel_cap_t b)
> +{
> +       unsigned __capi;
> +       CAP_FOR_EACH_U32(__capi) {
> +               if (a.cap[__capi] != b.cap[__capi])
> +                       return false;
> +       }
> +       return true;
> +}
> +

Side note, and this is not really related to this particular patch
other than because it just brought up the issue once more..

Our "kernel_cap_t" thing is disgusting.

It's been a structure containing

        __u32 cap[_KERNEL_CAPABILITY_U32S];

basically forever, and it's not likely to change in the future. I
would object to any crazy capability expansion, considering how
useless and painful they've been anyway, and I don't think anybody
really is even remotely planning anything like that anyway.

And what is _KERNEL_CAPABILITY_U32S anyway? It's the "third version"
of that size:

  #define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_3

which happens to be the same number as the second version of said
#define, which happens to be "2".

In other words, that fancy array is just 64 bits. And we'd probably be
better off just treating it as such, and just doing

        typedef u64 kernel_cap_t;

since we have to do the special "convert from user space format"
_anyway_, and this isn't something that is shared to user space as-is.

Then that "cap_isidentical()" would literally be just "a == b" instead
of us playing games with for-loops that are just two wide, and a
compiler that may or may not realize.

It would literally remove some of the insanity in <linux/capability.h>
- look for CAP_TO_MASK() and CAP_TO_INDEX and CAP_FS_MASK_B0 and
CAP_FS_MASK_B1 and just plain ugliness that comes from this entirely
historical oddity.

Yes, yes, we started out having it be a single-word array, and yes,
the code is written to think that it might some day be expanded past
the two words it then in 2008 it expanded to two words and 64 bits.
And now, fifteen years later, we use 40 of those 64 bits, and
hopefully we'll never add another one.

So we have historical reasons for why our kernel_cap_t is so odd. But
it *is* odd.

Hmm?

             Linus
  

On 2/27/2023 5:14 PM, Linus Torvalds wrote:
> On Wed, Jan 25, 2023 at 7:56 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
>> +static inline bool cap_isidentical(const kernel_cap_t a, const kernel_cap_t b)
>> +{
>> +       unsigned __capi;
>> +       CAP_FOR_EACH_U32(__capi) {
>> +               if (a.cap[__capi] != b.cap[__capi])
>> +                       return false;
>> +       }
>> +       return true;
>> +}
>> +
> Side note, and this is not really related to this particular patch
> other than because it just brought up the issue once more..
>
> Our "kernel_cap_t" thing is disgusting.
>
> It's been a structure containing
>
>         __u32 cap[_KERNEL_CAPABILITY_U32S];
>
> basically forever, and it's not likely to change in the future. I
> would object to any crazy capability expansion, considering how
> useless and painful they've been anyway, and I don't think anybody
> really is even remotely planning anything like that anyway.
>
> And what is _KERNEL_CAPABILITY_U32S anyway? It's the "third version"
> of that size:
>
>   #define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_3
>
> which happens to be the same number as the second version of said
> #define, which happens to be "2".
>
> In other words, that fancy array is just 64 bits. And we'd probably be
> better off just treating it as such, and just doing
>
>         typedef u64 kernel_cap_t;
>
> since we have to do the special "convert from user space format"
> _anyway_, and this isn't something that is shared to user space as-is.
>
> Then that "cap_isidentical()" would literally be just "a == b" instead
> of us playing games with for-loops that are just two wide, and a
> compiler that may or may not realize.
>
> It would literally remove some of the insanity in <linux/capability.h>
> - look for CAP_TO_MASK() and CAP_TO_INDEX and CAP_FS_MASK_B0 and
> CAP_FS_MASK_B1 and just plain ugliness that comes from this entirely
> historical oddity.
>
> Yes, yes, we started out having it be a single-word array, and yes,
> the code is written to think that it might some day be expanded past
> the two words it then in 2008 it expanded to two words and 64 bits.
> And now, fifteen years later, we use 40 of those 64 bits, and
> hopefully we'll never add another one.

I agree that the addition of 24 more capabilities is unlikely. The
two reasons presented recently for adding capabilities are to implement
boutique policies (CAP_MYHARDWAREISSPECIAL) or to break up CAP_SYS_ADMIN.
Neither of these is sustainable with a finite number of capabilities, nor
do they fit the security model capabilities implement. It's possible that
a small number of additional capabilities will be approved, but even that
seems unlikely.


> So we have historical reasons for why our kernel_cap_t is so odd. But
> it *is* odd.
>
> Hmm?

I don't see any reason that kernel_cap_t shouldn't be a u64. If by some
amazing change in mindset we develop need for 65 capabilities, someone can
dredge up the old code, shout "I told you so!" and put it back the way it
was. Or maybe by then we'll have u128, and can just switch to that.

>              Linus
  

On 2/28/23, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 2/27/2023 5:14 PM, Linus Torvalds wrote:
>> On Wed, Jan 25, 2023 at 7:56 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
>>> +static inline bool cap_isidentical(const kernel_cap_t a, const
>>> kernel_cap_t b)
>>> +{
>>> +       unsigned __capi;
>>> +       CAP_FOR_EACH_U32(__capi) {
>>> +               if (a.cap[__capi] != b.cap[__capi])
>>> +                       return false;
>>> +       }
>>> +       return true;
>>> +}
>>> +
>> Side note, and this is not really related to this particular patch
>> other than because it just brought up the issue once more..
>>
>> Our "kernel_cap_t" thing is disgusting.
>>
>> It's been a structure containing
>>
>>         __u32 cap[_KERNEL_CAPABILITY_U32S];
>>
>> basically forever, and it's not likely to change in the future. I
>> would object to any crazy capability expansion, considering how
>> useless and painful they've been anyway, and I don't think anybody
>> really is even remotely planning anything like that anyway.
>>
>> And what is _KERNEL_CAPABILITY_U32S anyway? It's the "third version"
>> of that size:
>>
>>   #define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_3
>>
>> which happens to be the same number as the second version of said
>> #define, which happens to be "2".
>>
>> In other words, that fancy array is just 64 bits. And we'd probably be
>> better off just treating it as such, and just doing
>>
>>         typedef u64 kernel_cap_t;
>>
>> since we have to do the special "convert from user space format"
>> _anyway_, and this isn't something that is shared to user space as-is.
>>
>> Then that "cap_isidentical()" would literally be just "a == b" instead
>> of us playing games with for-loops that are just two wide, and a
>> compiler that may or may not realize.
>>
>> It would literally remove some of the insanity in <linux/capability.h>
>> - look for CAP_TO_MASK() and CAP_TO_INDEX and CAP_FS_MASK_B0 and
>> CAP_FS_MASK_B1 and just plain ugliness that comes from this entirely
>> historical oddity.
>>
>> Yes, yes, we started out having it be a single-word array, and yes,
>> the code is written to think that it might some day be expanded past
>> the two words it then in 2008 it expanded to two words and 64 bits.
>> And now, fifteen years later, we use 40 of those 64 bits, and
>> hopefully we'll never add another one.
>
> I agree that the addition of 24 more capabilities is unlikely. The
> two reasons presented recently for adding capabilities are to implement
> boutique policies (CAP_MYHARDWAREISSPECIAL) or to break up CAP_SYS_ADMIN.
> Neither of these is sustainable with a finite number of capabilities, nor
> do they fit the security model capabilities implement. It's possible that
> a small number of additional capabilities will be approved, but even that
> seems unlikely.
>
>
>> So we have historical reasons for why our kernel_cap_t is so odd. But
>> it *is* odd.
>>
>> Hmm?
>
> I don't see any reason that kernel_cap_t shouldn't be a u64. If by some
> amazing change in mindset we develop need for 65 capabilities, someone can
> dredge up the old code, shout "I told you so!" and put it back the way it
> was. Or maybe by then we'll have u128, and can just switch to that.
>

Premature generalization is the root of all evil (or however the
saying goes), as evidenced above.

The fact that this is an array of u32 escaped the confines of
capability.h and as a result there would be unpleasant churn to sort
it out, and more importantly this requires a lot more testing than you
would normally expect.

Personally I would only touch it as a result of losing a bet (and I'm
not taking any with this in play), but that's just my $0.05 (adjusted
for inflation).
  

On Mon, Feb 27, 2023 at 06:46:12PM -0800, Casey Schaufler wrote:
> On 2/27/2023 5:14 PM, Linus Torvalds wrote:
> > On Wed, Jan 25, 2023 at 7:56 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
> >> +static inline bool cap_isidentical(const kernel_cap_t a, const kernel_cap_t b)
> >> +{
> >> +       unsigned __capi;
> >> +       CAP_FOR_EACH_U32(__capi) {
> >> +               if (a.cap[__capi] != b.cap[__capi])
> >> +                       return false;
> >> +       }
> >> +       return true;
> >> +}
> >> +
> > Side note, and this is not really related to this particular patch
> > other than because it just brought up the issue once more..
> >
> > Our "kernel_cap_t" thing is disgusting.
> >
> > It's been a structure containing
> >
> >         __u32 cap[_KERNEL_CAPABILITY_U32S];
> >
> > basically forever, and it's not likely to change in the future. I
> > would object to any crazy capability expansion, considering how
> > useless and painful they've been anyway, and I don't think anybody
> > really is even remotely planning anything like that anyway.
> >
> > And what is _KERNEL_CAPABILITY_U32S anyway? It's the "third version"
> > of that size:
> >
> >   #define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_3
> >
> > which happens to be the same number as the second version of said
> > #define, which happens to be "2".
> >
> > In other words, that fancy array is just 64 bits. And we'd probably be
> > better off just treating it as such, and just doing
> >
> >         typedef u64 kernel_cap_t;
> >
> > since we have to do the special "convert from user space format"
> > _anyway_, and this isn't something that is shared to user space as-is.
> >
> > Then that "cap_isidentical()" would literally be just "a == b" instead
> > of us playing games with for-loops that are just two wide, and a
> > compiler that may or may not realize.
> >
> > It would literally remove some of the insanity in <linux/capability.h>
> > - look for CAP_TO_MASK() and CAP_TO_INDEX and CAP_FS_MASK_B0 and
> > CAP_FS_MASK_B1 and just plain ugliness that comes from this entirely
> > historical oddity.
> >
> > Yes, yes, we started out having it be a single-word array, and yes,
> > the code is written to think that it might some day be expanded past
> > the two words it then in 2008 it expanded to two words and 64 bits.
> > And now, fifteen years later, we use 40 of those 64 bits, and
> > hopefully we'll never add another one.
> 
> I agree that the addition of 24 more capabilities is unlikely. The
> two reasons presented recently for adding capabilities are to implement
> boutique policies (CAP_MYHARDWAREISSPECIAL) or to break up CAP_SYS_ADMIN.

FWIW IMO breaking up CAP_SYS_ADMIN is a good thing, so long as we continue
to do it in the "you can use either CAP_SYS_ADMIN or CAP_NEW_FOO" way.  

But there haven't been many such patchsets :)

> Neither of these is sustainable with a finite number of capabilities, nor
> do they fit the security model capabilities implement. It's possible that
> a small number of additional capabilities will be approved, but even that
> seems unlikely.
> 
> 
> > So we have historical reasons for why our kernel_cap_t is so odd. But
> > it *is* odd.
> >
> > Hmm?
> 
> I don't see any reason that kernel_cap_t shouldn't be a u64. If by some
> amazing change in mindset we develop need for 65 capabilities, someone can
> dredge up the old code, shout "I told you so!" and put it back the way it
> was. Or maybe by then we'll have u128, and can just switch to that.
> 
> >              Linus
  

On 2/28/2023 9:32 AM, Serge E. Hallyn wrote:
> On Mon, Feb 27, 2023 at 06:46:12PM -0800, Casey Schaufler wrote:
>> On 2/27/2023 5:14 PM, Linus Torvalds wrote:
>>> On Wed, Jan 25, 2023 at 7:56 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
>>>> +static inline bool cap_isidentical(const kernel_cap_t a, const kernel_cap_t b)
>>>> +{
>>>> +       unsigned __capi;
>>>> +       CAP_FOR_EACH_U32(__capi) {
>>>> +               if (a.cap[__capi] != b.cap[__capi])
>>>> +                       return false;
>>>> +       }
>>>> +       return true;
>>>> +}
>>>> +
>>> Side note, and this is not really related to this particular patch
>>> other than because it just brought up the issue once more..
>>>
>>> Our "kernel_cap_t" thing is disgusting.
>>>
>>> It's been a structure containing
>>>
>>>         __u32 cap[_KERNEL_CAPABILITY_U32S];
>>>
>>> basically forever, and it's not likely to change in the future. I
>>> would object to any crazy capability expansion, considering how
>>> useless and painful they've been anyway, and I don't think anybody
>>> really is even remotely planning anything like that anyway.
>>>
>>> And what is _KERNEL_CAPABILITY_U32S anyway? It's the "third version"
>>> of that size:
>>>
>>>   #define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_3
>>>
>>> which happens to be the same number as the second version of said
>>> #define, which happens to be "2".
>>>
>>> In other words, that fancy array is just 64 bits. And we'd probably be
>>> better off just treating it as such, and just doing
>>>
>>>         typedef u64 kernel_cap_t;
>>>
>>> since we have to do the special "convert from user space format"
>>> _anyway_, and this isn't something that is shared to user space as-is.
>>>
>>> Then that "cap_isidentical()" would literally be just "a == b" instead
>>> of us playing games with for-loops that are just two wide, and a
>>> compiler that may or may not realize.
>>>
>>> It would literally remove some of the insanity in <linux/capability.h>
>>> - look for CAP_TO_MASK() and CAP_TO_INDEX and CAP_FS_MASK_B0 and
>>> CAP_FS_MASK_B1 and just plain ugliness that comes from this entirely
>>> historical oddity.
>>>
>>> Yes, yes, we started out having it be a single-word array, and yes,
>>> the code is written to think that it might some day be expanded past
>>> the two words it then in 2008 it expanded to two words and 64 bits.
>>> And now, fifteen years later, we use 40 of those 64 bits, and
>>> hopefully we'll never add another one.
>> I agree that the addition of 24 more capabilities is unlikely. The
>> two reasons presented recently for adding capabilities are to implement
>> boutique policies (CAP_MYHARDWAREISSPECIAL) or to break up CAP_SYS_ADMIN.
> FWIW IMO breaking up CAP_SYS_ADMIN is a good thing, so long as we continue
> to do it in the "you can use either CAP_SYS_ADMIN or CAP_NEW_FOO" way.  

You need to have a security policy to reference to add a capability.
Telling the disc to spin in the opposite direction, while important
to control, is not something that will fall under a security policy.
It is also rare for programs to need CAP_SYS_ADMIN for only one thing.

While I agree that we shouldn't be allowing a program to reverse the
spin of a drive just because it needs to adjust memory use on a network
interface, I don't believe that capabilities are the right approach.
Capabilities haven't proven popular for their intended purpose, so I
don't see them as a good candidate for extension. There were good reasons
for capabilities to work the way they do, but they have not all stood
the test of time. I do have a proposed implementation, but it's stuck
behind LSM stacking.

>
> But there haven't been many such patchsets :)
>
>> Neither of these is sustainable with a finite number of capabilities, nor
>> do they fit the security model capabilities implement. It's possible that
>> a small number of additional capabilities will be approved, but even that
>> seems unlikely.
>>
>>
>>> So we have historical reasons for why our kernel_cap_t is so odd. But
>>> it *is* odd.
>>>
>>> Hmm?
>> I don't see any reason that kernel_cap_t shouldn't be a u64. If by some
>> amazing change in mindset we develop need for 65 capabilities, someone can
>> dredge up the old code, shout "I told you so!" and put it back the way it
>> was. Or maybe by then we'll have u128, and can just switch to that.
>>
>>>              Linus
  

On Tue, Feb 28, 2023 at 6:47 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> Personally I would only touch it as a result of losing a bet (and I'm
> not taking any with this in play), but that's just my $0.05 (adjusted
> for inflation).

Heh. I took that as a challenge.

It wasn't actually all that bad (famous last words). For type safety
reasons I decided to use a struct wrapper

   typedef struct { u64 val; } kernel_cap_t;

to avoid any nasty silent integer value conversions. But then it was
literally just a matter of cleaning up some of the insanity.

I think the fs/proc/array.c modification is an example of just how bad
things used to be:

    --- a/fs/proc/array.c
    +++ b/fs/proc/array.c
    @@ -300,13 +300,8 @@ static inline void task_sig(struct seq_file *m,
     static void render_cap_t(struct seq_file *m, const char *header,
                            kernel_cap_t *a)
     {
    -       unsigned __capi;
    -
            seq_puts(m, header);
    -       CAP_FOR_EACH_U32(__capi) {
    -               seq_put_hex_ll(m, NULL,
    -                          a->cap[CAP_LAST_U32 - __capi], 8);
    -       }
    +       seq_put_hex_ll(m, NULL, a->val, 16);
            seq_putc(m, '\n');
     }

note how the code literally did that odd

        CAP_LAST_U32 - __capi

in there just to get the natural "high word first" that is exactly
what you get if you just print out the value as a hex number.

Now, the actual user mode conversions still exist, but even those
often got simplified.

Have I actually *tested* this? Of course not. I'm lazy, and everything
I write obviously always works on the first try anyway, so why should
I?

So take this patch with a healthy dose of salt, but it looks sane to
me, and it does build cleanly (and with our type system, that actually
does say quite a lot).

This actually looks sane enough that I might even boot it. Call me crazy.

           Linus
  

On Tue, Feb 28, 2023 at 11:39 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> This actually looks sane enough that I might even boot it. Call me crazy.

Oh, and while I haven't actually booted it or tested it in any way, I
did verify that it changes

-       movq    48(%rbx), %rax
-       movq    56(%rbx), %rcx
-       cmpl    %eax, %ecx
-       jne     .LBB58_13
-       shrq    $32, %rax
-       shrq    $32, %rcx
-       cmpl    %eax, %ecx
-       jne     .LBB58_13

into

+       movq    56(%rbx), %rax
+       cmpq    48(%rbx), %rax
+       jne     .LBB58_12

because it looks like clang was smart enough to unroll the silly
fixed-size loop and do the two adjacent 32-bit loads of the old cap[]
array as one 64-bit load, but then was _not_ smart enough to combine
the two 32-bit compares into one 64-bit one.

And gcc didn't do the load optimization (which is questionable since
it then just results in extra shifts and extra registers), so it just
kept it as two 32-bit loads and compares. Again, with the patch, gcc
obviously does the sane "one 64-bit load, one 64-bit compare" too.

There's a lot to be said for compiler optimizations fixing up silly
source code, but I personally would just prefer to make the source
code DTRT.

Could the compiler have been even smarter and generated the same code?
Yes it could. We shouldn't expect that, though. Particularly when the
sane code is much more legible to humans too.

                 Linus
  

On Tue, Feb 28, 2023 at 11:39 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> This actually looks sane enough that I might even boot it. Call me crazy.

LOL.

I had to go through the patch with a find comb, because everything
worked except for some reason network name resolution failed:
systemd-resolved got a permission error on

    Failed to listen on UDP socket 127.0.0.53:53: Permission denied

Spot the insufficient fixup in my cut-and-paste capget() patch:

  kdata[0].effective   = pE.val;
        kdata[1].effective   = pE.val >> 32;
  kdata[0].permitted   = pP.val;
        kdata[1].permitted   = pP.val >> 32;
  kdata[0].inheritable = pI.val;
        kdata[0].inheritable = pI.val >> 32;

Oops.

But with that fixed, that patch actually does seem to work.

             Linus
  

On 2/28/23, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Tue, Feb 28, 2023 at 11:39 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Call me crazy.
>

Hello crazy,

> I had to go through the patch with a find comb, because everything
> worked except for some reason network name resolution failed:
> systemd-resolved got a permission error on
>
>     Failed to listen on UDP socket 127.0.0.53:53: Permission denied
>
> Spot the insufficient fixup in my cut-and-paste capget() patch:
>
>   kdata[0].effective   = pE.val;
>         kdata[1].effective   = pE.val >> 32;
>   kdata[0].permitted   = pP.val;
>         kdata[1].permitted   = pP.val >> 32;
>   kdata[0].inheritable = pI.val;
>         kdata[0].inheritable = pI.val >> 32;
>
> Oops.
>
> But with that fixed, that patch actually does seem to work.
>

This is part of the crap which made me unwilling to do the clean up.

Unless there is a test suite (which I'm guessing there is not), I
think this warrants a prog which iterates over all methods with a
bunch of randomly generated capsets (+ maybe handpicked corner cases?)
and compares results new vs old. Otherwise I would feel very uneasy
signing off on the patch.

That said, nice cleanup if it works out :)
  

On Tue, Feb 28, 2023 at 1:21 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> This is part of the crap which made me unwilling to do the clean up.

Yeah, it's not pretty.

That said, the old code was worse. The only redeeming feature of the
old code was that "nobody has touched it in ages", so it was at least
stable.

              Linus
  

On Tue, Feb 28, 2023 at 1:29 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> That said, the old code was worse. The only redeeming feature of the
> old code was that "nobody has touched it in ages", so it was at least
> stable.

Bah. I've walked through that patch something like ten times now, and
decided that there's no way it breaks anything. Famous last words.

It also means that I don't want to look at that ugly old code when I
have the fix for it all, so I just moved it over from my experimental
tree to the main tree, since it's still the merge window.

Quod licet Iovi, non licet bovi, or something.

                 Linus