| Message ID | 20250123214342.4145818-1-andrii@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm,procfs: allow read-only remote mm access under CAP_PERFMON | expand |
On Thu, Jan 23, 2025 at 1:44 PM Andrii Nakryiko <andrii@kernel.org> wrote: > > It's very common for various tracing and profiling toolis to need to > access /proc/PID/maps contents for stack symbolization needs to learn > which shared libraries are mapped in memory, at which file offset, etc. > Currently, access to /proc/PID/maps requires CAP_SYS_PTRACE (unless we > are looking at data for our own process, which is a trivial case not too > relevant for profilers use cases). > > Unfortunately, CAP_SYS_PTRACE implies way more than just ability to > discover memory layout of another process: it allows to fully control > arbitrary other processes. This is problematic from security POV for > applications that only need read-only /proc/PID/maps (and other similar > read-only data) access, and in large production settings CAP_SYS_PTRACE > is frowned upon even for the system-wide profilers. > > On the other hand, it's already possible to access similar kind of > information (and more) with just CAP_PERFMON capability. E.g., setting > up PERF_RECORD_MMAP collection through perf_event_open() would give one > similar information to what /proc/PID/maps provides. > > CAP_PERFMON, together with CAP_BPF, is already a very common combination > for system-wide profiling and observability application. As such, it's > reasonable and convenient to be able to access /proc/PID/maps with > CAP_PERFMON capabilities instead of CAP_SYS_PTRACE. > > For procfs, these permissions are checked through common mm_access() > helper, and so we augment that with cap_perfmon() check *only* if > requested mode is PTRACE_MODE_READ. I.e., PTRACE_MODE_ATTACH wouldn't be > permitted by CAP_PERFMON. > > Besides procfs itself, mm_access() is used by process_madvise() and > process_vm_{readv,writev}() syscalls. The former one uses > PTRACE_MODE_READ to avoid leaking ASLR metadata, and as such CAP_PERFMON > seems like a meaningful allowable capability as well. > > process_vm_{readv,writev} currently assume PTRACE_MODE_ATTACH level of > permissions (though for readv PTRACE_MODE_READ seems more reasonable, > but that's outside the scope of this change), and as such won't be > affected by this patch. CC'ing Jann and Kees. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > kernel/fork.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/kernel/fork.c b/kernel/fork.c > index ded49f18cd95..c57cb3ad9931 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1547,6 +1547,15 @@ struct mm_struct *get_task_mm(struct task_struct *task) > } > EXPORT_SYMBOL_GPL(get_task_mm); > > +static bool can_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode) > +{ > + if (mm == current->mm) > + return true; > + if ((mode & PTRACE_MODE_READ) && perfmon_capable()) > + return true; > + return ptrace_may_access(task, mode); > +} > + > struct mm_struct *mm_access(struct task_struct *task, unsigned int mode) > { > struct mm_struct *mm; > @@ -1559,7 +1568,7 @@ struct mm_struct *mm_access(struct task_struct *task, unsigned int mode) > mm = get_task_mm(task); > if (!mm) { > mm = ERR_PTR(-ESRCH); > - } else if (mm != current->mm && !ptrace_may_access(task, mode)) { > + } else if (!can_access_mm(mm, task, mode)) { > mmput(mm); > mm = ERR_PTR(-EACCES); > } > -- > 2.43.5 >
On Thu, Jan 23, 2025 at 01:52:52PM -0800, Suren Baghdasaryan wrote: > On Thu, Jan 23, 2025 at 1:44 PM Andrii Nakryiko <andrii@kernel.org> wrote: > > > > It's very common for various tracing and profiling toolis to need to > > access /proc/PID/maps contents for stack symbolization needs to learn > > which shared libraries are mapped in memory, at which file offset, etc. > > Currently, access to /proc/PID/maps requires CAP_SYS_PTRACE (unless we > > are looking at data for our own process, which is a trivial case not too > > relevant for profilers use cases). > > > > Unfortunately, CAP_SYS_PTRACE implies way more than just ability to > > discover memory layout of another process: it allows to fully control > > arbitrary other processes. This is problematic from security POV for > > applications that only need read-only /proc/PID/maps (and other similar > > read-only data) access, and in large production settings CAP_SYS_PTRACE > > is frowned upon even for the system-wide profilers. > > > > On the other hand, it's already possible to access similar kind of > > information (and more) with just CAP_PERFMON capability. E.g., setting > > up PERF_RECORD_MMAP collection through perf_event_open() would give one > > similar information to what /proc/PID/maps provides. > > > > CAP_PERFMON, together with CAP_BPF, is already a very common combination > > for system-wide profiling and observability application. As such, it's > > reasonable and convenient to be able to access /proc/PID/maps with > > CAP_PERFMON capabilities instead of CAP_SYS_PTRACE. > > > > For procfs, these permissions are checked through common mm_access() > > helper, and so we augment that with cap_perfmon() check *only* if > > requested mode is PTRACE_MODE_READ. I.e., PTRACE_MODE_ATTACH wouldn't be > > permitted by CAP_PERFMON. > > > > Besides procfs itself, mm_access() is used by process_madvise() and > > process_vm_{readv,writev}() syscalls. The former one uses > > PTRACE_MODE_READ to avoid leaking ASLR metadata, and as such CAP_PERFMON > > seems like a meaningful allowable capability as well. > > > > process_vm_{readv,writev} currently assume PTRACE_MODE_ATTACH level of > > permissions (though for readv PTRACE_MODE_READ seems more reasonable, > > but that's outside the scope of this change), and as such won't be > > affected by this patch. > > CC'ing Jann and Kees. > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > kernel/fork.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/fork.c b/kernel/fork.c > > index ded49f18cd95..c57cb3ad9931 100644 > > --- a/kernel/fork.c > > +++ b/kernel/fork.c > > @@ -1547,6 +1547,15 @@ struct mm_struct *get_task_mm(struct task_struct *task) > > } > > EXPORT_SYMBOL_GPL(get_task_mm); > > > > +static bool can_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode) > > +{ > > + if (mm == current->mm) > > + return true; > > + if ((mode & PTRACE_MODE_READ) && perfmon_capable()) > > + return true; > > + return ptrace_may_access(task, mode); > > +} nit: "may" tends to be used more than "can" for access check function naming. So, this will bypass security_ptrace_access_check() within ptrace_may_access(). CAP_PERFMON may be something LSMs want visibility into. It also bypasses the dumpability check in __ptrace_may_access(). (Should non-dumpability block visibility into "maps" under CAP_PERFMON?) This change provides read access for CAP_PERFMON to: /proc/$pid/maps /proc/$pid/smaps /proc/$pid/mem /proc/$pid/environ /proc/$pid/auxv /proc/$pid/attr/* /proc/$pid/smaps_rollup /proc/$pid/pagemap /proc/$pid/mem access seems way out of bounds for CAP_PERFMON. environ and auxv maybe too much also. The "attr" files seem iffy. pagemap may be reasonable. Gaining CAP_PERFMON access to *only* the "maps" file doesn't seem too bad to me, but I think the proposed patch ends up providing way too wide access to other things. Also, this is doing an init-namespace capability check for CAP_PERFMON (via perfmon_capable()). Shouldn't this be per-namespace? -Kees > > + > > struct mm_struct *mm_access(struct task_struct *task, unsigned int mode) > > { > > struct mm_struct *mm; > > @@ -1559,7 +1568,7 @@ struct mm_struct *mm_access(struct task_struct *task, unsigned int mode) > > mm = get_task_mm(task); > > if (!mm) { > > mm = ERR_PTR(-ESRCH); > > - } else if (mm != current->mm && !ptrace_may_access(task, mode)) { > > + } else if (!can_access_mm(mm, task, mode)) { > > mmput(mm); > > mm = ERR_PTR(-EACCES); > > } > > -- > > 2.43.5 > >
On Fri, Jan 24, 2025 at 12:47 AM Kees Cook <kees@kernel.org> wrote: > On Thu, Jan 23, 2025 at 01:52:52PM -0800, Suren Baghdasaryan wrote: > > On Thu, Jan 23, 2025 at 1:44 PM Andrii Nakryiko <andrii@kernel.org> wrote: > > > > > > It's very common for various tracing and profiling toolis to need to > > > access /proc/PID/maps contents for stack symbolization needs to learn > > > which shared libraries are mapped in memory, at which file offset, etc. > > > Currently, access to /proc/PID/maps requires CAP_SYS_PTRACE (unless we > > > are looking at data for our own process, which is a trivial case not too > > > relevant for profilers use cases). > > > > > > Unfortunately, CAP_SYS_PTRACE implies way more than just ability to > > > discover memory layout of another process: it allows to fully control > > > arbitrary other processes. This is problematic from security POV for > > > applications that only need read-only /proc/PID/maps (and other similar > > > read-only data) access, and in large production settings CAP_SYS_PTRACE > > > is frowned upon even for the system-wide profilers. > > > > > > On the other hand, it's already possible to access similar kind of > > > information (and more) with just CAP_PERFMON capability. E.g., setting > > > up PERF_RECORD_MMAP collection through perf_event_open() would give one > > > similar information to what /proc/PID/maps provides. > > > > > > CAP_PERFMON, together with CAP_BPF, is already a very common combination > > > for system-wide profiling and observability application. As such, it's > > > reasonable and convenient to be able to access /proc/PID/maps with > > > CAP_PERFMON capabilities instead of CAP_SYS_PTRACE. > > > > > > For procfs, these permissions are checked through common mm_access() > > > helper, and so we augment that with cap_perfmon() check *only* if > > > requested mode is PTRACE_MODE_READ. I.e., PTRACE_MODE_ATTACH wouldn't be > > > permitted by CAP_PERFMON. > > > > > > Besides procfs itself, mm_access() is used by process_madvise() and > > > process_vm_{readv,writev}() syscalls. The former one uses > > > PTRACE_MODE_READ to avoid leaking ASLR metadata, and as such CAP_PERFMON > > > seems like a meaningful allowable capability as well. > > > > > > process_vm_{readv,writev} currently assume PTRACE_MODE_ATTACH level of > > > permissions (though for readv PTRACE_MODE_READ seems more reasonable, > > > but that's outside the scope of this change), and as such won't be > > > affected by this patch. > > > > CC'ing Jann and Kees. > > > > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > --- > > > kernel/fork.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/fork.c b/kernel/fork.c > > > index ded49f18cd95..c57cb3ad9931 100644 > > > --- a/kernel/fork.c > > > +++ b/kernel/fork.c > > > @@ -1547,6 +1547,15 @@ struct mm_struct *get_task_mm(struct task_struct *task) > > > } > > > EXPORT_SYMBOL_GPL(get_task_mm); > > > > > > +static bool can_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode) > > > +{ > > > + if (mm == current->mm) > > > + return true; > > > + if ((mode & PTRACE_MODE_READ) && perfmon_capable()) > > > + return true; > > > + return ptrace_may_access(task, mode); > > > +} > > nit: "may" tends to be used more than "can" for access check function naming. > > So, this will bypass security_ptrace_access_check() within > ptrace_may_access(). CAP_PERFMON may be something LSMs want visibility > into. > > It also bypasses the dumpability check in __ptrace_may_access(). (Should > non-dumpability block visibility into "maps" under CAP_PERFMON?) > > This change provides read access for CAP_PERFMON to: > > /proc/$pid/maps > /proc/$pid/smaps > /proc/$pid/mem > /proc/$pid/environ > /proc/$pid/auxv > /proc/$pid/attr/* > /proc/$pid/smaps_rollup > /proc/$pid/pagemap > > /proc/$pid/mem access seems way out of bounds for CAP_PERFMON. environ > and auxv maybe too much also. The "attr" files seem iffy. pagemap may be > reasonable. FWIW, my understanding is that if you can use perf_event_open() on a process, you can also grab large amounts of stack memory contents from that process via PERF_SAMPLE_STACK_USER/sample_stack_user. (The idea there is that stack unwinding for userspace stacks is complicated, so it's the profiler's job to turn a pile of raw stack contents and a register snapshot into a stack trace.) So _to some extent_ I think it is already possible to read memory of another process via CAP_PERFMON. Whether that is desirable or not I don't know, though I guess it's hard to argue that there's a qualitative security difference between reading register contents and reading stack memory...
On Thu, Jan 23, 2025 at 03:47:44PM -0800, Kees Cook wrote: > On Thu, Jan 23, 2025 at 01:52:52PM -0800, Suren Baghdasaryan wrote: > > On Thu, Jan 23, 2025 at 1:44 PM Andrii Nakryiko <andrii@kernel.org> wrote: > > > > > > It's very common for various tracing and profiling toolis to need to > > > access /proc/PID/maps contents for stack symbolization needs to learn > > > which shared libraries are mapped in memory, at which file offset, etc. > > > Currently, access to /proc/PID/maps requires CAP_SYS_PTRACE (unless we > > > are looking at data for our own process, which is a trivial case not too > > > relevant for profilers use cases). > > > > > > Unfortunately, CAP_SYS_PTRACE implies way more than just ability to > > > discover memory layout of another process: it allows to fully control > > > arbitrary other processes. This is problematic from security POV for > > > applications that only need read-only /proc/PID/maps (and other similar > > > read-only data) access, and in large production settings CAP_SYS_PTRACE > > > is frowned upon even for the system-wide profilers. > > > > > > On the other hand, it's already possible to access similar kind of > > > information (and more) with just CAP_PERFMON capability. E.g., setting > > > up PERF_RECORD_MMAP collection through perf_event_open() would give one > > > similar information to what /proc/PID/maps provides. > > > > > > CAP_PERFMON, together with CAP_BPF, is already a very common combination > > > for system-wide profiling and observability application. As such, it's > > > reasonable and convenient to be able to access /proc/PID/maps with > > > CAP_PERFMON capabilities instead of CAP_SYS_PTRACE. > > > > > > For procfs, these permissions are checked through common mm_access() > > > helper, and so we augment that with cap_perfmon() check *only* if > > > requested mode is PTRACE_MODE_READ. I.e., PTRACE_MODE_ATTACH wouldn't be > > > permitted by CAP_PERFMON. > > > > > > Besides procfs itself, mm_access() is used by process_madvise() and > > > process_vm_{readv,writev}() syscalls. The former one uses > > > PTRACE_MODE_READ to avoid leaking ASLR metadata, and as such CAP_PERFMON > > > seems like a meaningful allowable capability as well. > > > > > > process_vm_{readv,writev} currently assume PTRACE_MODE_ATTACH level of > > > permissions (though for readv PTRACE_MODE_READ seems more reasonable, > > > but that's outside the scope of this change), and as such won't be > > > affected by this patch. > > > > CC'ing Jann and Kees. > > > > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > --- > > > kernel/fork.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/fork.c b/kernel/fork.c > > > index ded49f18cd95..c57cb3ad9931 100644 > > > --- a/kernel/fork.c > > > +++ b/kernel/fork.c > > > @@ -1547,6 +1547,15 @@ struct mm_struct *get_task_mm(struct task_struct *task) > > > } > > > EXPORT_SYMBOL_GPL(get_task_mm); > > > > > > +static bool can_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode) > > > +{ > > > + if (mm == current->mm) > > > + return true; > > > + if ((mode & PTRACE_MODE_READ) && perfmon_capable()) > > > + return true; > > > + return ptrace_may_access(task, mode); > > > +} > > nit: "may" tends to be used more than "can" for access check function naming. > > So, this will bypass security_ptrace_access_check() within > ptrace_may_access(). CAP_PERFMON may be something LSMs want visibility > into. > > It also bypasses the dumpability check in __ptrace_may_access(). (Should > non-dumpability block visibility into "maps" under CAP_PERFMON?) > > This change provides read access for CAP_PERFMON to: > > /proc/$pid/maps > /proc/$pid/smaps > /proc/$pid/mem > /proc/$pid/environ > /proc/$pid/auxv > /proc/$pid/attr/* > /proc/$pid/smaps_rollup > /proc/$pid/pagemap > > /proc/$pid/mem access seems way out of bounds for CAP_PERFMON. environ > and auxv maybe too much also. The "attr" files seem iffy. pagemap may be > reasonable. From what I understand, PTRACE_MODE_ATTACH is used for /proc/$pid/mem, so this patch is not changing anything. However for environ and auxv, PTRACE_MODE_READ is being used, so they will be accessible for CAP_PERFMON. What's your reason behind too much for environ and auxv?
On Thu, Jan 23, 2025 at 3:47 PM Kees Cook <kees@kernel.org> wrote: > > On Thu, Jan 23, 2025 at 01:52:52PM -0800, Suren Baghdasaryan wrote: > > On Thu, Jan 23, 2025 at 1:44 PM Andrii Nakryiko <andrii@kernel.org> wrote: > > > > > > It's very common for various tracing and profiling toolis to need to > > > access /proc/PID/maps contents for stack symbolization needs to learn > > > which shared libraries are mapped in memory, at which file offset, etc. > > > Currently, access to /proc/PID/maps requires CAP_SYS_PTRACE (unless we > > > are looking at data for our own process, which is a trivial case not too > > > relevant for profilers use cases). > > > > > > Unfortunately, CAP_SYS_PTRACE implies way more than just ability to > > > discover memory layout of another process: it allows to fully control > > > arbitrary other processes. This is problematic from security POV for > > > applications that only need read-only /proc/PID/maps (and other similar > > > read-only data) access, and in large production settings CAP_SYS_PTRACE > > > is frowned upon even for the system-wide profilers. > > > > > > On the other hand, it's already possible to access similar kind of > > > information (and more) with just CAP_PERFMON capability. E.g., setting > > > up PERF_RECORD_MMAP collection through perf_event_open() would give one > > > similar information to what /proc/PID/maps provides. > > > > > > CAP_PERFMON, together with CAP_BPF, is already a very common combination > > > for system-wide profiling and observability application. As such, it's > > > reasonable and convenient to be able to access /proc/PID/maps with > > > CAP_PERFMON capabilities instead of CAP_SYS_PTRACE. > > > > > > For procfs, these permissions are checked through common mm_access() > > > helper, and so we augment that with cap_perfmon() check *only* if > > > requested mode is PTRACE_MODE_READ. I.e., PTRACE_MODE_ATTACH wouldn't be > > > permitted by CAP_PERFMON. > > > > > > Besides procfs itself, mm_access() is used by process_madvise() and > > > process_vm_{readv,writev}() syscalls. The former one uses > > > PTRACE_MODE_READ to avoid leaking ASLR metadata, and as such CAP_PERFMON > > > seems like a meaningful allowable capability as well. > > > > > > process_vm_{readv,writev} currently assume PTRACE_MODE_ATTACH level of > > > permissions (though for readv PTRACE_MODE_READ seems more reasonable, > > > but that's outside the scope of this change), and as such won't be > > > affected by this patch. > > > > CC'ing Jann and Kees. > > > > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > --- > > > kernel/fork.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/fork.c b/kernel/fork.c > > > index ded49f18cd95..c57cb3ad9931 100644 > > > --- a/kernel/fork.c > > > +++ b/kernel/fork.c > > > @@ -1547,6 +1547,15 @@ struct mm_struct *get_task_mm(struct task_struct *task) > > > } > > > EXPORT_SYMBOL_GPL(get_task_mm); > > > > > > +static bool can_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode) > > > +{ > > > + if (mm == current->mm) > > > + return true; > > > + if ((mode & PTRACE_MODE_READ) && perfmon_capable()) > > > + return true; > > > + return ptrace_may_access(task, mode); > > > +} > > nit: "may" tends to be used more than "can" for access check function naming. good point, will change to "may" > > So, this will bypass security_ptrace_access_check() within > ptrace_may_access(). CAP_PERFMON may be something LSMs want visibility > into. yeah, similar to perf's perf_check_permission() (though, admittedly, perf has its own security_perf_event_open(&attr, PERF_SECURITY_OPEN) check much earlier in perf_event_open() logic) > > It also bypasses the dumpability check in __ptrace_may_access(). (Should > non-dumpability block visibility into "maps" under CAP_PERFMON?) With perf_event_open() and PERF_RECORD_MMAP none of this dumpability is honored today as well, so I think CAP_PERFMON should override all these ptrace things here, no? > > This change provides read access for CAP_PERFMON to: > > /proc/$pid/maps > /proc/$pid/smaps > /proc/$pid/mem > /proc/$pid/environ > /proc/$pid/auxv > /proc/$pid/attr/* > /proc/$pid/smaps_rollup > /proc/$pid/pagemap > > /proc/$pid/mem access seems way out of bounds for CAP_PERFMON. environ > and auxv maybe too much also. The "attr" files seem iffy. pagemap may be > reasonable. As Shakeel pointed out, /proc/PID/mem is PTRACE_MODE_ATTACH, so won't be permitted under CAP_PERFMON either. Don't really know what auxv is, but I could read all that with BPF if I had CAP_PERFMON, for any task, so not like we are opening up new possibilities here. > > Gaining CAP_PERFMON access to *only* the "maps" file doesn't seem too > bad to me, but I think the proposed patch ends up providing way too wide > access to other things. I do care about maps mostly, yes, but I also wanted to avoid duplicating all that mm_access() logic just for maps (and probably smaps, they are the same data). But again, CAP_PERFMON basically means read-only tracing access to anything within kernel and any user process, so it felt appropriate to allow CAP_PERFMON here. > > Also, this is doing an init-namespace capability check for > CAP_PERFMON (via perfmon_capable()). Shouldn't this be per-namespace? CAP_PERFMON isn't namespaced as far as perf_event_open() is concerned, so at least for that reason I don't want to relax the requirement here. Namespacing CAP_PERFMON in general is interesting and I bet there are users that would appreciate that, but that's an entire epic journey we probably don't want to start here. > > -Kees > > > > + > > > struct mm_struct *mm_access(struct task_struct *task, unsigned int mode) > > > { > > > struct mm_struct *mm; > > > @@ -1559,7 +1568,7 @@ struct mm_struct *mm_access(struct task_struct *task, unsigned int mode) > > > mm = get_task_mm(task); > > > if (!mm) { > > > mm = ERR_PTR(-ESRCH); > > > - } else if (mm != current->mm && !ptrace_may_access(task, mode)) { > > > + } else if (!can_access_mm(mm, task, mode)) { > > > mmput(mm); > > > mm = ERR_PTR(-EACCES); > > > } > > > -- > > > 2.43.5 > > > > > -- > Kees Cook
On Thu, Jan 23, 2025 at 3:55 PM Jann Horn <jannh@google.com> wrote: > > On Fri, Jan 24, 2025 at 12:47 AM Kees Cook <kees@kernel.org> wrote: > > On Thu, Jan 23, 2025 at 01:52:52PM -0800, Suren Baghdasaryan wrote: > > > On Thu, Jan 23, 2025 at 1:44 PM Andrii Nakryiko <andrii@kernel.org> wrote: > > > > > > > > It's very common for various tracing and profiling toolis to need to > > > > access /proc/PID/maps contents for stack symbolization needs to learn > > > > which shared libraries are mapped in memory, at which file offset, etc. > > > > Currently, access to /proc/PID/maps requires CAP_SYS_PTRACE (unless we > > > > are looking at data for our own process, which is a trivial case not too > > > > relevant for profilers use cases). > > > > > > > > Unfortunately, CAP_SYS_PTRACE implies way more than just ability to > > > > discover memory layout of another process: it allows to fully control > > > > arbitrary other processes. This is problematic from security POV for > > > > applications that only need read-only /proc/PID/maps (and other similar > > > > read-only data) access, and in large production settings CAP_SYS_PTRACE > > > > is frowned upon even for the system-wide profilers. > > > > > > > > On the other hand, it's already possible to access similar kind of > > > > information (and more) with just CAP_PERFMON capability. E.g., setting > > > > up PERF_RECORD_MMAP collection through perf_event_open() would give one > > > > similar information to what /proc/PID/maps provides. > > > > > > > > CAP_PERFMON, together with CAP_BPF, is already a very common combination > > > > for system-wide profiling and observability application. As such, it's > > > > reasonable and convenient to be able to access /proc/PID/maps with > > > > CAP_PERFMON capabilities instead of CAP_SYS_PTRACE. > > > > > > > > For procfs, these permissions are checked through common mm_access() > > > > helper, and so we augment that with cap_perfmon() check *only* if > > > > requested mode is PTRACE_MODE_READ. I.e., PTRACE_MODE_ATTACH wouldn't be > > > > permitted by CAP_PERFMON. > > > > > > > > Besides procfs itself, mm_access() is used by process_madvise() and > > > > process_vm_{readv,writev}() syscalls. The former one uses > > > > PTRACE_MODE_READ to avoid leaking ASLR metadata, and as such CAP_PERFMON > > > > seems like a meaningful allowable capability as well. > > > > > > > > process_vm_{readv,writev} currently assume PTRACE_MODE_ATTACH level of > > > > permissions (though for readv PTRACE_MODE_READ seems more reasonable, > > > > but that's outside the scope of this change), and as such won't be > > > > affected by this patch. > > > > > > CC'ing Jann and Kees. > > > > > > > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > > --- > > > > kernel/fork.c | 11 ++++++++++- > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/fork.c b/kernel/fork.c > > > > index ded49f18cd95..c57cb3ad9931 100644 > > > > --- a/kernel/fork.c > > > > +++ b/kernel/fork.c > > > > @@ -1547,6 +1547,15 @@ struct mm_struct *get_task_mm(struct task_struct *task) > > > > } > > > > EXPORT_SYMBOL_GPL(get_task_mm); > > > > > > > > +static bool can_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode) > > > > +{ > > > > + if (mm == current->mm) > > > > + return true; > > > > + if ((mode & PTRACE_MODE_READ) && perfmon_capable()) > > > > + return true; > > > > + return ptrace_may_access(task, mode); > > > > +} > > > > nit: "may" tends to be used more than "can" for access check function naming. > > > > So, this will bypass security_ptrace_access_check() within > > ptrace_may_access(). CAP_PERFMON may be something LSMs want visibility > > into. > > > > It also bypasses the dumpability check in __ptrace_may_access(). (Should > > non-dumpability block visibility into "maps" under CAP_PERFMON?) > > > > This change provides read access for CAP_PERFMON to: > > > > /proc/$pid/maps > > /proc/$pid/smaps > > /proc/$pid/mem > > /proc/$pid/environ > > /proc/$pid/auxv > > /proc/$pid/attr/* > > /proc/$pid/smaps_rollup > > /proc/$pid/pagemap > > > > /proc/$pid/mem access seems way out of bounds for CAP_PERFMON. environ > > and auxv maybe too much also. The "attr" files seem iffy. pagemap may be > > reasonable. > > FWIW, my understanding is that if you can use perf_event_open() on a > process, you can also grab large amounts of stack memory contents from > that process via PERF_SAMPLE_STACK_USER/sample_stack_user. (The idea > there is that stack unwinding for userspace stacks is complicated, so > it's the profiler's job to turn a pile of raw stack contents and a > register snapshot into a stack trace.) So _to some extent_ I think it > is already possible to read memory of another process via CAP_PERFMON. > Whether that is desirable or not I don't know, though I guess it's > hard to argue that there's a qualitative security difference between > reading register contents and reading stack memory... If I'm allowed to bring in BPF capabilities coupled with CAP_PERFMON, then you can read not just stack, but pretty much anything both inside the kernel memory (e.g., through bpf_probe_read_kernel()) and user-space (bpf_probe_read_user() for current user task, and more generally bpf_copy_from_user_task() for an arbitrary task for which we have struct task_struct). But we don't really allow access to /proc/PID/mem here, because it's PTRACE_MODE_ATTACH (which is sort of like read/write vs read-only). Similarly, it would be relevant for process_vm_readv(), but that one (currently) is also PTRACE_MODE_ATTACH.
diff --git a/kernel/fork.c b/kernel/fork.c index ded49f18cd95..c57cb3ad9931 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1547,6 +1547,15 @@ struct mm_struct *get_task_mm(struct task_struct *task) } EXPORT_SYMBOL_GPL(get_task_mm); +static bool can_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode) +{ + if (mm == current->mm) + return true; + if ((mode & PTRACE_MODE_READ) && perfmon_capable()) + return true; + return ptrace_may_access(task, mode); +} + struct mm_struct *mm_access(struct task_struct *task, unsigned int mode) { struct mm_struct *mm; @@ -1559,7 +1568,7 @@ struct mm_struct *mm_access(struct task_struct *task, unsigned int mode) mm = get_task_mm(task); if (!mm) { mm = ERR_PTR(-ESRCH); - } else if (mm != current->mm && !ptrace_may_access(task, mode)) { + } else if (!can_access_mm(mm, task, mode)) { mmput(mm); mm = ERR_PTR(-EACCES); }
It's very common for various tracing and profiling toolis to need to access /proc/PID/maps contents for stack symbolization needs to learn which shared libraries are mapped in memory, at which file offset, etc. Currently, access to /proc/PID/maps requires CAP_SYS_PTRACE (unless we are looking at data for our own process, which is a trivial case not too relevant for profilers use cases). Unfortunately, CAP_SYS_PTRACE implies way more than just ability to discover memory layout of another process: it allows to fully control arbitrary other processes. This is problematic from security POV for applications that only need read-only /proc/PID/maps (and other similar read-only data) access, and in large production settings CAP_SYS_PTRACE is frowned upon even for the system-wide profilers. On the other hand, it's already possible to access similar kind of information (and more) with just CAP_PERFMON capability. E.g., setting up PERF_RECORD_MMAP collection through perf_event_open() would give one similar information to what /proc/PID/maps provides. CAP_PERFMON, together with CAP_BPF, is already a very common combination for system-wide profiling and observability application. As such, it's reasonable and convenient to be able to access /proc/PID/maps with CAP_PERFMON capabilities instead of CAP_SYS_PTRACE. For procfs, these permissions are checked through common mm_access() helper, and so we augment that with cap_perfmon() check *only* if requested mode is PTRACE_MODE_READ. I.e., PTRACE_MODE_ATTACH wouldn't be permitted by CAP_PERFMON. Besides procfs itself, mm_access() is used by process_madvise() and process_vm_{readv,writev}() syscalls. The former one uses PTRACE_MODE_READ to avoid leaking ASLR metadata, and as such CAP_PERFMON seems like a meaningful allowable capability as well. process_vm_{readv,writev} currently assume PTRACE_MODE_ATTACH level of permissions (though for readv PTRACE_MODE_READ seems more reasonable, but that's outside the scope of this change), and as such won't be affected by this patch. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- kernel/fork.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)