Re: [PATCH v2] mm_access: simplify the security checks

[David Hildenbrand (Arm)]

On 5/30/26 16:12, Oleg Nesterov wrote:

I am not sure I spot the "simplification" here?

Looks more like an optimization that makes the code slightly more complicated.

> 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@xxxxxxxxxx>
> ---
>  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)

We now have a function named "may_access_mm" and not providing an mm struct
pointer ... it now looks more like an extended, mm-independent ptrace check.

So probably we should rename that function.

>  {
> -	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;


-- 
Cheers,

David