Messages in this thread | | | Date | Sat, 14 Jan 2012 18:58:29 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH] Fix race in process_vm_rw_core |
| |
On 01/14, Christopher Yeoh wrote: > > On Fri, 13 Jan 2012 17:04:42 +0100 > Oleg Nesterov <oleg@redhat.com> wrote: > > On 01/13, Christopher Yeoh wrote: > > > ... > > > +struct mm_struct *get_check_task_mm(struct task_struct *task, > > > unsigned int mode) +{ > > > + struct mm_struct *mm; > > > + int err; > > > + > > > + err = > > > mutex_lock_killable(&task->signal->cred_guard_mutex); > > > + if (err) > > > + return ERR_PTR(err); > > > + > > > + task_lock(task); > > > + if (__ptrace_may_access(task, mode)) { > > > + mm = ERR_PTR(-EACCES); > > > + goto out; > > > + } > > > > Probably you should check "mm != current->mm" before > > __ptrace_may_access(), otherwise this changes the rules for, > > say, /proc/pid/maps. > > __ptrace_may_access has a check for task == current already - > Is that sufficient? > > /* Don't let security modules deny introspection */ > if (task == current) > return 0;
I don't think this is sufficient in the multithreaded or CLONE_VM case, task_cred/etc is per-thread.
It is not that I think that this "current->mm != mm" check is important, in fact personally I think it shouldn't exist.
But we shouldn't add the subtle and not documented behavioural change, and obviously process_vm_rw() has no security problems if mm == current->mm.
> > > + mm = get_check_task_mm(task, PTRACE_MODE_ATTACH); > > > + if (!mm || IS_ERR(mm)) { > > > + if (!mm) > > > + rc = -EINVAL; > > > + else > > > + rc = -EPERM; > > > > Cosmetic nit. I won't insist, but why -EPERM is better than -EACCES > > returned by get_check_task_mm()? IOW, why not rc = PTR_ERR() ? > > Maybe I should just convert EACCES to EPERM for process_vm_rw_core. I > left get_check_task_mm with EACCESS to preserve existing behaviour > for mm_for_maps. > > SUSv3 defines EACCES and EPERM as > > [EACCES] > Permission denied. An attempt was made to access a file in a way > forbidden by its file access permissions. > > [EPERM] > Operation not permitted. An attempt was made to perform an operation > limited to processes with appropriate privileges or to the owner of a > file or other resource. > > So EPERM is more appropriate for process_vm_readv/writev
Well, imho EACCES would be fine too and my point was s/EINTR/EPERM/ looks a bit confusing.
But OK, this is subjective and minor, I won't argue.
Oleg.
| |