Skip to contentSkip to conversation

mm: relax ptrace mode in process_vm_readv(2)

PATCH In review Alexey Dobriyan 2 participants 3 messages Mar 3, 2018 – Mar 6, 2018

Subsystems (2)
Memory management MM misc
lore
Share & tools
lore

Alexey Dobriyan

author
#0 lore raw

PATCH 1/1 mm: relax ptrace mode in process_vm_readv(2)

It is more natural to check for read-from-memory permissions in case of process_vm_readv() as PTRACE_MODE_ATTACH is equivalent to write permissions.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 mm/process_vm_access.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/process_vm_access.c +1 -1
@@ -204,7 +204,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
 		goto free_proc_pages;
 	}
 
-	mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS);
+	mm = mm_access(task, vm_write ? PTRACE_MODE_ATTACH_REALCREDS : PTRACE_MODE_READ_REALCREDS);
 	if (!mm || IS_ERR(mm)) {
 		rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
 		/*
maintainer
#1 lore raw
Quoted 9 lines · On Sat, 3 Mar 2018 23:11:16 +0300 Alexey Dobriyan <[email protected]> wrote:

It is more natural to check for read-from-memory permissions in case of process_vm_readv() as PTRACE_MODE_ATTACH is equivalent to write permissions.

...

mm/process_vm_access.c static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter, · line 210

Quoted 8 lines
mm/process_vm_access.c +1 -1
@@ -204,7 +204,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
 		goto free_proc_pages;
 	}
 
-	mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS);
+	mm = mm_access(task, vm_write ? PTRACE_MODE_ATTACH_REALCREDS : PTRACE_MODE_READ_REALCREDS);
 	if (!mm || IS_ERR(mm)) {
 		rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
 		/*

But what is the risk of breaking existing userspace?

author
#2 lore raw
Quoted 14 lines · Andrew Morton
Quoted 12 lines · On Sat, 3 Mar 2018 23:11:16 +0300 Alexey Dobriyan <[email protected]> wrote:

It is more natural to check for read-from-memory permissions in case of process_vm_readv() as PTRACE_MODE_ATTACH is equivalent to write permissions.

...

mm/process_vm_access.c +0 -0
@@ -204,7 +204,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
 		goto free_proc_pages;
 	}
Quoted 7 lines

mm/process_vm_access.c static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter, · line 210

Quoted 5 lines
mm/process_vm_access.c +1 -1
-	mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS);
+	mm = mm_access(task, vm_write ? PTRACE_MODE_ATTACH_REALCREDS : PTRACE_MODE_READ_REALCREDS);
 	if (!mm || IS_ERR(mm)) {
 		rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
 		/*

But what is the risk of breaking existing userspace?

Permissions for write/ATTACH should be more strict than for read/READ, so loosening them should be fine.

Unless LSM does silly things of course.