| Message ID | 20230125155557.37816-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v3,1/2] capability: add cap_isidentical | expand |
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
diff --git a/include/linux/capability.h b/include/linux/capability.h index 65efb74c3585..736a973c677a 100644 --- a/include/linux/capability.h +++ b/include/linux/capability.h @@ -156,6 +156,16 @@ static inline bool cap_isclear(const kernel_cap_t a) return true; } +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; +} + /* * Check if "a" is a subset of "set". * return true if ALL of the capabilities in "a" are also in "set"