Re: [rfc 3/3] prctl: Add PR_SET_MM codes to tune up mm_struct entires

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Tue, Nov 29, 2011 at 12:19:38PM -0800, Kees Cook wrote:
> On Tue, Nov 29, 2011 at 11:12:55PM +0400, Cyrill Gorcunov wrote:
> > At restore time we need a mechanism to restore those values
> > back and for this sake PR_SET_MM prctl code is introduced.
> > 
> > Note at moment this inteface is allowed for CAP_SYS_ADMIN
> > only.
> 
> NAK from me; this needs more bounds checking. Though, yes, it absolutely
> must be a privileged action since this is potentially very dangerous. Can
> we invent something stronger than CAP_SYS_ADMIN? ;)

Heh.

> 
> > @@ -1841,6 +1841,58 @@ SYSCALL_DEFINE5(prctl, int, option, unsi
> >  			else
> >  				error = PR_MCE_KILL_DEFAULT;
> >  			break;
> > +		case PR_SET_MM: {
> > +			struct mm_struct *mm;
> > +			struct vm_area_struct *vma;
> > +
> > +			if (arg4 | arg5)
> > +				return -EINVAL;
> > +
> > +			if (!capable(CAP_SYS_ADMIN))
> > +				return -EPERM;
> > +
> > +			error = -ENOENT;
> > +			mm = get_task_mm(current);
> > +			if (!mm)
> > +				return error;
> > +
> > +			down_read(&mm->mmap_sem);
> > +			vma = find_vma(mm, arg3);
> > +			if (!vma)
> > +				goto out;
> 
> arg3 needs to be significantly more carefully validated. find_vma() doesn't
> say that vm_start <= addr, only that vm_end > addr. This effectively
> bypasses all the vma checks (mmap_min_addr, max process size, etc), with
> some pretty crazy side-effects, I think.
> 

Yes, I know it needs some more testing, but apart from vma bounds (yup,
good point with find_vma, I'll fix) I thought about what else should be
checked? I think VMA prototype should be checked to fit "code", "data"
templates, ie code should be at least readable and execytable, but what
about data and stack and brk, should stack be executable? That is the
point where I've got a bit confused and though putting RFC out might be
a good idea to collect opinions.