* [PATCH] Unify migrate_pages and move_pages access checks
@ 2017-10-01 15:33 Otto Ebeling
2017-10-04 14:00 ` Michal Hocko
0 siblings, 1 reply; 2+ messages in thread
From: Otto Ebeling @ 2017-10-01 15:33 UTC (permalink / raw)
To: linux-mm; +Cc: Christoph Lameter
Commit 197e7e521384a23b9e585178f3f11c9fa08274b9 ("Sanitize 'move_pages()'
permission checks") fixed a security issue I reported in the move_pages
syscall, and made it so that you can't act on set-uid processes unless
you have the CAP_SYS_PTRACE capability.
Unify the access check logic of migrate_pages to match the new
behavior of move_pages. We discussed this a bit in the security@ list
and thought it'd be good for consistency even though there's no evident
security impact. The NUMA node access checks are left intact and require
CAP_SYS_NICE as before.
Signed-off-by: Otto Ebeling <otto.ebeling@iki.fi>
---
mm/mempolicy.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 006ba62..abfe469 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -98,6 +98,7 @@
#include <linux/mmu_notifier.h>
#include <linux/printk.h>
#include <linux/swapops.h>
+#include <linux/ptrace.h>
#include <asm/tlbflush.h>
#include <linux/uaccess.h>
@@ -1365,7 +1366,6 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned
long, maxnode,
const unsigned long __user *, old_nodes,
const unsigned long __user *, new_nodes)
{
- const struct cred *cred = current_cred(), *tcred;
struct mm_struct *mm = NULL;
struct task_struct *task;
nodemask_t task_nodes;
@@ -1402,14 +1402,9 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned
long, maxnode,
/*
* Check if this process has the right to modify the specified
- * process. The right exists if the process has administrative
- * capabilities, superuser privileges or the same
- * userid as the target process.
+ * process. Use the regular "ptrace_may_access()" checks.
*/
- tcred = __task_cred(task);
- if (!uid_eq(cred->euid, tcred->suid) && !uid_eq(cred->euid,
tcred->uid) &&
- !uid_eq(cred->uid, tcred->suid) && !uid_eq(cred->uid,
tcred->uid) &&
- !capable(CAP_SYS_NICE)) {
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
rcu_read_unlock();
err = -EPERM;
goto out_put;
--
2.1.4
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] Unify migrate_pages and move_pages access checks
2017-10-01 15:33 [PATCH] Unify migrate_pages and move_pages access checks Otto Ebeling
@ 2017-10-04 14:00 ` Michal Hocko
0 siblings, 0 replies; 2+ messages in thread
From: Michal Hocko @ 2017-10-04 14:00 UTC (permalink / raw)
To: Otto Ebeling; +Cc: linux-mm, Christoph Lameter
On Sun 01-10-17 18:33:39, Otto Ebeling wrote:
> Commit 197e7e521384a23b9e585178f3f11c9fa08274b9 ("Sanitize 'move_pages()'
> permission checks") fixed a security issue I reported in the move_pages
> syscall, and made it so that you can't act on set-uid processes unless
> you have the CAP_SYS_PTRACE capability.
>
> Unify the access check logic of migrate_pages to match the new
> behavior of move_pages. We discussed this a bit in the security@ list
> and thought it'd be good for consistency even though there's no evident
> security impact. The NUMA node access checks are left intact and require
> CAP_SYS_NICE as before.
>
> Signed-off-by: Otto Ebeling <otto.ebeling@iki.fi>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/mempolicy.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 006ba62..abfe469 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -98,6 +98,7 @@
> #include <linux/mmu_notifier.h>
> #include <linux/printk.h>
> #include <linux/swapops.h>
> +#include <linux/ptrace.h>
>
> #include <asm/tlbflush.h>
> #include <linux/uaccess.h>
> @@ -1365,7 +1366,6 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned
> long, maxnode,
> const unsigned long __user *, old_nodes,
> const unsigned long __user *, new_nodes)
> {
> - const struct cred *cred = current_cred(), *tcred;
> struct mm_struct *mm = NULL;
> struct task_struct *task;
> nodemask_t task_nodes;
> @@ -1402,14 +1402,9 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned
> long, maxnode,
>
> /*
> * Check if this process has the right to modify the specified
> - * process. The right exists if the process has administrative
> - * capabilities, superuser privileges or the same
> - * userid as the target process.
> + * process. Use the regular "ptrace_may_access()" checks.
> */
> - tcred = __task_cred(task);
> - if (!uid_eq(cred->euid, tcred->suid) && !uid_eq(cred->euid, tcred->uid) &&
> - !uid_eq(cred->uid, tcred->suid) && !uid_eq(cred->uid, tcred->uid) &&
> - !capable(CAP_SYS_NICE)) {
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
> rcu_read_unlock();
> err = -EPERM;
> goto out_put;
> --
> 2.1.4
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-10-04 14:00 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-01 15:33 [PATCH] Unify migrate_pages and move_pages access checks Otto Ebeling
2017-10-04 14:00 ` Michal Hocko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).