On 04.03.21 01:03, Shakeel Butt wrote:
On Wed, Mar 3, 2021 at 3:34 PM Suren Baghdasaryan surenb@google.com wrote:
On Wed, Mar 3, 2021 at 3:17 PM Shakeel Butt shakeelb@google.com wrote:
On Wed, Mar 3, 2021 at 10:58 AM Suren Baghdasaryan surenb@google.com wrote:
process_madvise currently requires ptrace attach capability. PTRACE_MODE_ATTACH gives one process complete control over another process. It effectively removes the security boundary between the two processes (in one direction). Granting ptrace attach capability even to a system process is considered dangerous since it creates an attack surface. This severely limits the usage of this API. The operations process_madvise can perform do not affect the correctness of the operation of the target process; they only affect where the data is physically located (and therefore, how fast it can be accessed). What we want is the ability for one process to influence another process in order to optimize performance across the entire system while leaving the security boundary intact. Replace PTRACE_MODE_ATTACH with a combination of PTRACE_MODE_READ and CAP_SYS_NICE. PTRACE_MODE_READ to prevent leaking ASLR metadata and CAP_SYS_NICE for influencing process performance.
Cc: stable@vger.kernel.org # 5.10+ Signed-off-by: Suren Baghdasaryan surenb@google.com Reviewed-by: Kees Cook keescook@chromium.org Acked-by: Minchan Kim minchan@kernel.org Acked-by: David Rientjes rientjes@google.com
changes in v3
Added Reviewed-by: Kees Cook keescook@chromium.org
Created man page for process_madvise per Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f...
cc'ed stable@vger.kernel.org # 5.10+ per Andrew's request
cc'ed linux-security-module@vger.kernel.org per James Morris's request
mm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/mm/madvise.c b/mm/madvise.c index df692d2e35d4..01fef79ac761 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -1198,12 +1198,22 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, goto release_task; }
mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
/* Require PTRACE_MODE_READ to avoid leaking ASLR metadata. */mm = mm_access(task, PTRACE_MODE_READ_FSCREDS); if (IS_ERR_OR_NULL(mm)) { ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH; goto release_task; }/** Require CAP_SYS_NICE for influencing process performance. Note that* only non-destructive hints are currently supported.How is non-destructive defined? Is MADV_DONTNEED non-destructive?
Non-destructive in this context means the data is not lost and can be recovered. I follow the logic described in https://lwn.net/Articles/794704/ where Minchan was introducing MADV_COLD and MADV_PAGEOUT as non-destructive versions of MADV_FREE and MADV_DONTNEED. Following that logic, MADV_FREE and MADV_DONTNEED would be considered destructive hints. Note that process_madvise_behavior_valid() allows only MADV_COLD and MADV_PAGEOUT at the moment, which are both non-destructive.
There is a plan to support MADV_DONTNEED for this syscall. Do we need to change these access checks again with that support?
Eh, I absolutely don't think letting another process discard memory in another process' address space is a good idea. The target process can observe that easily and might even run into real issues.
What's the use case?