lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 14 Jan 2012 18:58:29 +0100 From: Oleg Nesterov <oleg@...hat.com> To: Christopher Yeoh <cyeoh@....ibm.com> Cc: linux-kernel@...r.kernel.org, Linus Torvalds <torvalds@...ux-foundation.org>, Andrew Morton <akpm@...ux-foundation.org>, David Howells <dhowells@...hat.com> 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@...hat.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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists