mm/madvise: replace ptrace attach requirement for process_madvise
Subsystems (6)
PATCH v3 1/1 mm/madvise: replace ptrace attach requirement for process_madvise
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.
---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page for process_madvise per Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's request
mm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
mm/madvise.c +12 -1
@@ -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. +*/ +if (!capable (CAP_SYS_NICE )) { + ret = -EPERM ; + goto release_mm; + } + total_len =iov_iter_count (&iter);while (iov_iter_count (&iter)) {
@@ -1218,6 +1228,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,if (ret ==0 ) ret = total_len -iov_iter_count (&iter); +release_mm:mmput (mm); release_task:put_task_struct (task);
Signature · 1 line
2.30.1.766.gb4fecdf3b7-goog
Quoted 41 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +0
-1@@ -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 );
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 10 lines
mm/madvise.c +5
-0+/* 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?
Quoted 20 lines
mm/madvise.c +7
-1+*/ +if (!capable (CAP_SYS_NICE )) { + ret = -EPERM ; + goto release_mm; + } + total_len =iov_iter_count (&iter);while (iov_iter_count (&iter)) {@@ -1218,6 +1228,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,if (ret ==0 ) ret = total_len -iov_iter_count (&iter); +release_mm:mmput (mm); release_task:put_task_struct (task); --2.30.1.766.gb4fecdf3b7-goog
Quoted 50 lines · Shakeel Butt
Quoted 48 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +2
-1@@ -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; }
Quoted 5 lines
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 3 lines
mm/madvise.c +3
-0+/* +* Require CAP_SYS_NICE for influencing process performance. Note that +* only non-destructive hints are currently supported. How is non-destructive defined? Is
MADV_DONTNEEDnon-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.
Quoted 21 lines
Quoted 21 lines
mm/madvise.c +7
-1+*/ +if (!capable (CAP_SYS_NICE )) { + ret = -EPERM ; + goto release_mm; + } + total_len =iov_iter_count (&iter);while (iov_iter_count (&iter)) {@@ -1218,6 +1228,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,if (ret ==0 ) ret = total_len -iov_iter_count (&iter); +release_mm:mmput (mm); release_task:put_task_struct (task); --2.30.1.766.gb4fecdf3b7-goog
Quoted 58 lines · Suren Baghdasaryan
Quoted 50 lines · Shakeel Butt
Quoted 48 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +2
-1@@ -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; }Quoted 5 lines
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 3 lines
mm/madvise.c +3
-0+/* +* Require CAP_SYS_NICE for influencing process performance. Note that +* only non-destructive hints are currently supported. How is non-destructive defined? Is
MADV_DONTNEEDnon-destructive?
Quoted 9 lines
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 introducingMADV_COLDandMADV_PAGEOUTas non-destructive versions ofMADV_FREEandMADV_DONTNEED. Following that logic,MADV_FREEandMADV_DONTNEEDwould be considered destructive hints.
Note thatprocess_madvise_behavior_valid()allows onlyMADV_COLDandMADV_PAGEOUTat 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?
Quoted 70 lines · Shakeel Butt
Quoted 58 lines · Suren Baghdasaryan
Quoted 50 lines · Shakeel Butt
Quoted 48 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +2
-1@@ -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; }Quoted 5 lines
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 3 lines
mm/madvise.c +3
-0+/* +* Require CAP_SYS_NICE for influencing process performance. Note that +* only non-destructive hints are currently supported. How is non-destructive defined? Is
MADV_DONTNEEDnon-destructive?Quoted 9 lines
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 introducingMADV_COLDandMADV_PAGEOUTas non-destructive versions ofMADV_FREEandMADV_DONTNEED. Following that logic,MADV_FREEandMADV_DONTNEEDwould be considered destructive hints.
Note thatprocess_madvise_behavior_valid()allows onlyMADV_COLDandMADV_PAGEOUTat the moment, which are both non-destructive.
Quoted 2 lines
There is a plan to support
MADV_DONTNEEDfor this syscall. Do we need to change these access checks again with that support?
I think so. Destructive hints affect the data, so we will probably need stricter checks for those hints.
Quoted 69 lines · Shakeel Butt
Quoted 58 lines · Suren Baghdasaryan
Quoted 50 lines · Shakeel Butt
Quoted 48 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +2
-1@@ -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; }Quoted 5 lines
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 3 lines
mm/madvise.c +3
-0+/* +* Require CAP_SYS_NICE for influencing process performance. Note that +* only non-destructive hints are currently supported. How is non-destructive defined? Is
MADV_DONTNEEDnon-destructive?Quoted 9 lines
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 introducingMADV_COLDandMADV_PAGEOUTas non-destructive versions ofMADV_FREEandMADV_DONTNEED. Following that logic,MADV_FREEandMADV_DONTNEEDwould be considered destructive hints.
Note thatprocess_madvise_behavior_valid()allows onlyMADV_COLDandMADV_PAGEOUTat the moment, which are both non-destructive.
Quoted 2 lines
There is a plan to support
MADV_DONTNEEDfor 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?
Signature · 3 lines
Thanks, David / dhildenb
Quoted 71 lines · David Hildenbrand
Quoted 69 lines · Shakeel Butt
Quoted 67 lines · Suren Baghdasaryan
Quoted 50 lines · Shakeel Butt
Quoted 48 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +2
-1@@ -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; }Quoted 5 lines
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 3 lines
mm/madvise.c +3
-0+/* +* Require CAP_SYS_NICE for influencing process performance. Note that +* only non-destructive hints are currently supported. How is non-destructive defined? Is
MADV_DONTNEEDnon-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 introducingMADV_COLDandMADV_PAGEOUTas non-destructive versions ofMADV_FREEandMADV_DONTNEED. Following that logic,MADV_FREEandMADV_DONTNEEDwould be considered destructive hints.
Note thatprocess_madvise_behavior_valid()allows onlyMADV_COLDandMADV_PAGEOUTat the moment, which are both non-destructive.
Quoted 9 lines
Quoted 2 lines
There is a plan to support
MADV_DONTNEEDfor 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?
Userspace oom reaper. Please look at
https://lore.kernel.org/linux-api/[email protected]/T/
Quoted 79 lines · Shakeel Butt
Quoted 78 lines · David Hildenbrand
Quoted 69 lines · Shakeel Butt
Quoted 58 lines · Suren Baghdasaryan
Quoted 50 lines · Shakeel Butt
Quoted 48 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +2
-1@@ -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; }Quoted 5 lines
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 3 lines
mm/madvise.c +3
-0+/* +* Require CAP_SYS_NICE for influencing process performance. Note that +* only non-destructive hints are currently supported. How is non-destructive defined? Is
MADV_DONTNEEDnon-destructive?Quoted 9 lines
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 introducingMADV_COLDandMADV_PAGEOUTas non-destructive versions ofMADV_FREEandMADV_DONTNEED. Following that logic,MADV_FREEandMADV_DONTNEEDwould be considered destructive hints.
Note thatprocess_madvise_behavior_valid()allows onlyMADV_COLDandMADV_PAGEOUTat the moment, which are both non-destructive.Quoted 2 lines
There is a plan to support
MADV_DONTNEEDfor 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.
Quoted 6 lines
Quoted 2 lines
What's the use case?
Userspace oom reaper. Please look at
https://lore.kernel.org/linux-api/[email protected]/T/
Thanks, somehow I missed that (not that it really changed my opinion on the approach while skimming over the discussion :) will have a more detailed look)
Signature · 3 lines
Thanks, David / dhildenb
Quoted 84 lines · David Hildenbrand
Quoted 82 lines · Shakeel Butt
Quoted 80 lines · David Hildenbrand
Quoted 69 lines · Shakeel Butt
Quoted 58 lines · Suren Baghdasaryan
Quoted 50 lines · Shakeel Butt
Quoted 48 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +2
-1@@ -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; }Quoted 5 lines
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 3 lines
mm/madvise.c +3
-0+/* +* Require CAP_SYS_NICE for influencing process performance. Note that +* only non-destructive hints are currently supported. How is non-destructive defined? Is
MADV_DONTNEEDnon-destructive?Quoted 9 lines
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 introducingMADV_COLDandMADV_PAGEOUTas non-destructive versions ofMADV_FREEandMADV_DONTNEED. Following that logic,MADV_FREEandMADV_DONTNEEDwould be considered destructive hints.
Note thatprocess_madvise_behavior_valid()allows onlyMADV_COLDandMADV_PAGEOUTat the moment, which are both non-destructive.Quoted 2 lines
There is a plan to support
MADV_DONTNEEDfor 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?
Quoted 7 lines
Quoted 3 lines
Userspace oom reaper. Please look at
https://lore.kernel.org/linux-api/[email protected]/T/Thanks, somehow I missed that (not that it really changed my opinion on the approach while skimming over the discussion :) will have a more detailed look)
The latest version of that patchset is:
https://lore.kernel.org/patchwork/patch/1344419/
Yeah, memory reaping is a special case when we are operating on a dying process to speed up the release of its memory. I don't know if for that particular case we need to make the checks stricter. It's a dying process anyway and the data is being destroyed. Allowing to speed up that process probably can still use CAP_SYS_NICE.
Quoted 6 lines
--
Thanks,David / dhildenb
Quoted 93 lines · Suren Baghdasaryan
Quoted 84 lines · David Hildenbrand
Quoted 82 lines · Shakeel Butt
Quoted 80 lines · David Hildenbrand
Quoted 69 lines · Shakeel Butt
Quoted 58 lines · Suren Baghdasaryan
Quoted 50 lines · Shakeel Butt
Quoted 48 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +2
-1@@ -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; }Quoted 5 lines
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 3 lines
mm/madvise.c +3
-0+/* +* Require CAP_SYS_NICE for influencing process performance. Note that +* only non-destructive hints are currently supported. How is non-destructive defined? Is
MADV_DONTNEEDnon-destructive?Quoted 9 lines
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 introducingMADV_COLDandMADV_PAGEOUTas non-destructive versions ofMADV_FREEandMADV_DONTNEED. Following that logic,MADV_FREEandMADV_DONTNEEDwould be considered destructive hints.
Note thatprocess_madvise_behavior_valid()allows onlyMADV_COLDandMADV_PAGEOUTat the moment, which are both non-destructive.Quoted 2 lines
There is a plan to support
MADV_DONTNEEDfor 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?
Quoted 7 lines
Quoted 3 lines
Userspace oom reaper. Please look at
https://lore.kernel.org/linux-api/[email protected]/T/Thanks, somehow I missed that (not that it really changed my opinion on the approach while skimming over the discussion :) will have a more detailed look)
Quoted 7 lines
The latest version of that patchset is:
https://lore.kernel.org/patchwork/patch/1344419/
Yeah, memory reaping is a special case when we are operating on a dying process to speed up the release of its memory. I don't know if for that particular case we need to make the checks stricter. It's a dying process anyway and the data is being destroyed. Allowing to speed up that process probably can still useCAP_SYS_NICE.
I know, unrelated discussion (sorry, I don't have above thread in my archive anymore due to automatic cleanups ...) , but introducing MADV_DONTEED on a remote processes, having to tweak range logic because we always want to apply it to the whole MM, just to speed up memory reaping sounds like completely abusing madvise()/process_madvise() to me.
You want different semantics than MADV_DONTNEED. You want different semantics than madvise.
Simple example: mlock()ed pages in the target process. MADV_DONTNEED would choke on that. For the use case of reaping, you certainly don't care.
I am not sure if process_madvise() is the right interface to enforce discarding of all target memory.
Not to mention that MADV_FREE doesn't make any sense IMHO for memory reaping ... no to mention exposing this via process_madvise().
Signature · 3 lines
Thanks, David / dhildenb
Quoted 115 lines · David Hildenbrand
Quoted 93 lines · Suren Baghdasaryan
Quoted 84 lines · David Hildenbrand
Quoted 82 lines · Shakeel Butt
Quoted 80 lines · David Hildenbrand
Quoted 69 lines · Shakeel Butt
Quoted 58 lines · Suren Baghdasaryan
Quoted 50 lines · Shakeel Butt
Quoted 48 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +2
-1@@ -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; }Quoted 5 lines
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 3 lines
mm/madvise.c +3
-0+/* +* Require CAP_SYS_NICE for influencing process performance. Note that +* only non-destructive hints are currently supported. How is non-destructive defined? Is
MADV_DONTNEEDnon-destructive?Quoted 9 lines
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 introducingMADV_COLDandMADV_PAGEOUTas non-destructive versions ofMADV_FREEandMADV_DONTNEED. Following that logic,MADV_FREEandMADV_DONTNEEDwould be considered destructive hints.
Note thatprocess_madvise_behavior_valid()allows onlyMADV_COLDandMADV_PAGEOUTat the moment, which are both non-destructive.Quoted 2 lines
There is a plan to support
MADV_DONTNEEDfor 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?
Quoted 7 lines
Quoted 3 lines
Userspace oom reaper. Please look at
https://lore.kernel.org/linux-api/[email protected]/T/Thanks, somehow I missed that (not that it really changed my opinion on the approach while skimming over the discussion :) will have a more detailed look)
Quoted 7 lines
The latest version of that patchset is:
https://lore.kernel.org/patchwork/patch/1344419/
Yeah, memory reaping is a special case when we are operating on a dying process to speed up the release of its memory. I don't know if for that particular case we need to make the checks stricter. It's a dying process anyway and the data is being destroyed. Allowing to speed up that process probably can still useCAP_SYS_NICE.I know, unrelated discussion (sorry, I don't have above thread in my archive anymore due to automatic cleanups ...) , but introducing
MADV_DONTEEDon a remote processes, having to tweak range logic because we always want to apply it to the whole MM, just to speed up memory reaping sounds like completely abusing madvise()/process_madvise() to me.You want different semantics than
MADV_DONTNEED. You want different semantics than madvise.Simple example: mlock()ed pages in the target process.
MADV_DONTNEEDwould choke on that. For the use case of reaping, you certainly don't care.
Quoted 6 lines
I am not sure if
process_madvise()is the right interface to enforce discarding of all target memory.Not to mention that
MADV_FREEdoesn't make any sense IMHO for memory reaping ... no to mention exposing this viaprocess_madvise().
Yeah, that was the last comment from Christoph Hellwig on
https://lore.kernel.org/patchwork/patch/1344418/
I'll be rethinking the whole approach. Previously I proposed couple different approaches that would make reaping a part of the kill by adding a new flag for pidfd_send_signal:
https://lore.kernel.org/patchwork/patch/1338196/
https://lore.kernel.org/patchwork/patch/1060407/
but maybe a separate syscall for reaping is indeed the right way to go...
Quoted 6 lines
--
Thanks,David / dhildenb
Quoted 126 lines
Am 05.03.2021 um 19:36 schrieb Suren Baghdasaryan <[email protected]>:
Quoted 115 lines · David Hildenbrand
Quoted 95 lines · Suren Baghdasaryan
Quoted 84 lines · David Hildenbrand
Quoted 82 lines · Shakeel Butt
Quoted 80 lines · David Hildenbrand
Quoted 69 lines · Shakeel Butt
Quoted 58 lines · Suren Baghdasaryan
Quoted 50 lines · Shakeel Butt
Quoted 48 lines · Suren Baghdasaryan
process_madvisecurrently requires ptrace attach capability.PTRACE_MODE_ATTACHgives 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 operationsprocess_madvisecan 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.
ReplacePTRACE_MODE_ATTACHwith a combination ofPTRACE_MODE_READandCAP_SYS_NICE.PTRACE_MODE_READto prevent leaking ASLR metadata andCAP_SYS_NICEfor influencing process performance.Cc [email protected] # 5.10+Signed-off-by Suren Baghdasaryan <[email protected]>Reviewed-by Kees Cook <[email protected]>Acked-by Minchan Kim <[email protected]>Acked-by David Rientjes <[email protected]>---
changes in v3
- Added Reviewed-by: Kees Cook <[email protected]>
- Created man page forprocess_madviseper Andrew's request: https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=a144f458bad476a3358e3a45023789cb7bb9f993
- cc'ed [email protected] # 5.10+ per Andrew's request
- cc'ed [email protected] per James Morris's requestmm/madvise.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)mm/madvise.c +2
-1@@ -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; }Quoted 5 lines
mm/madvise.c SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, · line 1210
Quoted 3 lines
mm/madvise.c +3
-0+/* +* Require CAP_SYS_NICE for influencing process performance. Note that +* only non-destructive hints are currently supported. How is non-destructive defined? Is
MADV_DONTNEEDnon-destructive?Quoted 9 lines
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 introducingMADV_COLDandMADV_PAGEOUTas non-destructive versions ofMADV_FREEandMADV_DONTNEED. Following that logic,MADV_FREEandMADV_DONTNEEDwould be considered destructive hints.
Note thatprocess_madvise_behavior_valid()allows onlyMADV_COLDandMADV_PAGEOUTat the moment, which are both non-destructive.Quoted 2 lines
There is a plan to support
MADV_DONTNEEDfor 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?
Quoted 7 lines
Quoted 3 lines
Userspace oom reaper. Please look at
https://lore.kernel.org/linux-api/[email protected]/T/Thanks, somehow I missed that (not that it really changed my opinion on the approach while skimming over the discussion :) will have a more detailed look)
Quoted 7 lines
The latest version of that patchset is:
https://lore.kernel.org/patchwork/patch/1344419/
Yeah, memory reaping is a special case when we are operating on a dying process to speed up the release of its memory. I don't know if for that particular case we need to make the checks stricter. It's a dying process anyway and the data is being destroyed. Allowing to speed up that process probably can still useCAP_SYS_NICE.I know, unrelated discussion (sorry, I don't have above thread in my archive anymore due to automatic cleanups ...) , but introducing
MADV_DONTEEDon a remote processes, having to tweak range logic because we always want to apply it to the whole MM, just to speed up memory reaping sounds like completely abusing madvise()/process_madvise() to me.You want different semantics than
MADV_DONTNEED. You want different semantics than madvise.Simple example: mlock()ed pages in the target process.
MADV_DONTNEEDwould choke on that. For the use case of reaping, you certainly don't care.Quoted 6 lines
I am not sure if
process_madvise()is the right interface to enforce discarding of all target memory.Not to mention that
MADV_FREEdoesn't make any sense IMHO for memory reaping ... no to mention exposing this viaprocess_madvise().
Quoted 8 lines
Yeah, that was the last comment from Christoph Hellwig on
https://lore.kernel.org/patchwork/patch/1344418/
I'll be rethinking the whole approach. Previously I proposed couple different approaches that would make reaping a part of the kill by adding a new flag forpidfd_send_signal:
https://lore.kernel.org/patchwork/patch/1338196/
https://lore.kernel.org/patchwork/patch/1060407/
but maybe a separate syscall for reaping is indeed the right way to go...
Yeah, most likely!