lkml.org 
[lkml]   [2012]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.



\
 
 \ /
  Last update: 2012-01-14 19:07    [W:1.575 / U:0.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site