[PATCH] mm_access: simplify the security checks

[PATCH] mm_access: simplify the security checks

[Oleg Nesterov]

1. Shift the fast-path "mm == current->mm" check from may_access_mm()
   to mm_access(), and do it locklessly.

   task->mm is not stable but we do not care. We can race with exec,
   but in this case we pin/return current->mm. This doesn't differ
   from the case where the target execs after we drop exec_update_lock.

   All we need for correctness is READ_ONCE() to ensure the compiler
   won't reload task->mm. This is not enough for KCSAN, but we already
   have other lockless ->mm LOAD's. We should probably change exec_mmap/
   exit_mm to use WRITE_ONCE().

2. With the change above may_access_mm() doesn't need the "mm" argument,
   so we do not need to call get_task_mm() beforehand, we can call it
   only if may_access_mm() suceeds.

2. With the change above, may_access_mm() doesn't need the "mm" argument,
   so we do not need to call get_task_mm() beforehand. We can call it
   only if may_access_mm() succeeds.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/fork.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index b8b651abce8b..3239380ab93b 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1381,10 +1381,8 @@ struct mm_struct *get_task_mm(struct task_struct *task)
 }
 EXPORT_SYMBOL_GPL(get_task_mm);
 
-static bool may_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode)
+static bool may_access_mm(struct task_struct *task, unsigned int mode)
 {
-	if (mm == current->mm)
-		return true;
 	if (ptrace_may_access(task, mode))
 		return true;
 	if ((mode & PTRACE_MODE_READ) && perfmon_capable())
@@ -1394,20 +1392,24 @@ static bool may_access_mm(struct mm_struct *mm, struct task_struct *task, unsign
 
 struct mm_struct *mm_access(struct task_struct *task, unsigned int mode)
 {
-	struct mm_struct *mm;
-	int err;
+	struct mm_struct *mm = READ_ONCE(task->mm);
 
-	err =  down_read_killable(&task->signal->exec_update_lock);
-	if (err)
-		return ERR_PTR(err);
+	if (!mm || (task->flags & PF_KTHREAD))
+		return ERR_PTR(-ESRCH);
 
-	mm = get_task_mm(task);
-	if (!mm) {
-		mm = ERR_PTR(-ESRCH);
-	} else if (!may_access_mm(mm, task, mode)) {
-		mmput(mm);
-		mm = ERR_PTR(-EACCES);
+	if (mm == current->mm) {
+		mmget(mm);
+		return mm;
 	}
+
+	if (down_read_killable(&task->signal->exec_update_lock))
+		return ERR_PTR(-EINTR);
+
+	if (may_access_mm(task, mode))
+		mm = get_task_mm(task) ?: ERR_PTR(-ESRCH);
+	else
+		mm = ERR_PTR(-EACCES);
+
 	up_read(&task->signal->exec_update_lock);
 
 	return mm;
-- 
2.52.0

Re: [PATCH] mm_access: simplify the security checks

[Oleg Nesterov]

On 05/30, Oleg Nesterov wrote:
>
> 2. With the change above may_access_mm() doesn't need the "mm" argument,
>    so we do not need to call get_task_mm() beforehand, we can call it
>    only if may_access_mm() suceeds.
>
> 2. With the change above, may_access_mm() doesn't need the "mm" argument,
>    so we do not need to call get_task_mm() beforehand. We can call it
>    only if may_access_mm() succeeds.

OOPS. Somehow I wrote the same twice. Will fix the changelog and send V2
in reply to this patch...

Oleg.

[PATCH v2] mm_access: simplify the security checks

[Oleg Nesterov]

1. Shift the fast-path "mm == current->mm" check from may_access_mm()
   to mm_access(), and do it locklessly.

   task->mm is not stable but we do not care. We can race with exec,
   but in this case we pin/return current->mm. This doesn't differ
   from the case where the target execs after we drop exec_update_lock.

   All we need for correctness is READ_ONCE() to ensure the compiler
   won't reload task->mm. This is not enough for KCSAN, but we already
   have other lockless ->mm LOAD's. We should probably change exec_mmap/
   exit_mm to use WRITE_ONCE().

2. With the change above, may_access_mm() doesn't need the "mm" argument,
   so we do not need to call get_task_mm() beforehand. We can call it
   only if may_access_mm() succeeds.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/fork.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index b8b651abce8b..3239380ab93b 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1381,10 +1381,8 @@ struct mm_struct *get_task_mm(struct task_struct *task)
 }
 EXPORT_SYMBOL_GPL(get_task_mm);
 
-static bool may_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode)
+static bool may_access_mm(struct task_struct *task, unsigned int mode)
 {
-	if (mm == current->mm)
-		return true;
 	if (ptrace_may_access(task, mode))
 		return true;
 	if ((mode & PTRACE_MODE_READ) && perfmon_capable())
@@ -1394,20 +1392,24 @@ static bool may_access_mm(struct mm_struct *mm, struct task_struct *task, unsign
 
 struct mm_struct *mm_access(struct task_struct *task, unsigned int mode)
 {
-	struct mm_struct *mm;
-	int err;
+	struct mm_struct *mm = READ_ONCE(task->mm);
 
-	err =  down_read_killable(&task->signal->exec_update_lock);
-	if (err)
-		return ERR_PTR(err);
+	if (!mm || (task->flags & PF_KTHREAD))
+		return ERR_PTR(-ESRCH);
 
-	mm = get_task_mm(task);
-	if (!mm) {
-		mm = ERR_PTR(-ESRCH);
-	} else if (!may_access_mm(mm, task, mode)) {
-		mmput(mm);
-		mm = ERR_PTR(-EACCES);
+	if (mm == current->mm) {
+		mmget(mm);
+		return mm;
 	}
+
+	if (down_read_killable(&task->signal->exec_update_lock))
+		return ERR_PTR(-EINTR);
+
+	if (may_access_mm(task, mode))
+		mm = get_task_mm(task) ?: ERR_PTR(-ESRCH);
+	else
+		mm = ERR_PTR(-EACCES);
+
 	up_read(&task->signal->exec_update_lock);
 
 	return mm;
-- 
2.52.0

Re: [PATCH] mm_access: simplify the security checks

[Oleg Nesterov]

On 05/30, Oleg Nesterov wrote:
>
> 1. Shift the fast-path "mm == current->mm" check from may_access_mm()
>    to mm_access(), and do it locklessly.

Heh ;) sashiko has some concerns, see
https://sashiko.dev/#/patchset/ahrsfJE3NkKjShEX%40redhat.com

I still think this is fine, but I'll recheck tomorrow.

Oleg.