| Message ID | Zztcp-fm9Ln57c-t@lei (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [GIT,PULL] capabilities | expand |
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
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 :)
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
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
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
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
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
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
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...
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.
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