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

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Wed, Nov 30, 2011 at 10:10:35AM -0800, Kees Cook wrote:
...
> >
> > Hi Kees,
> >
> > what about this one? Note that these mm_struct members don't affect
> > kernel much (at least as far as I see, except maybe brk,start_brk and
> > start_stack values), so I've added some sanity checks here, hope they
> > would fit. Still main protection is root-only access only. The kernel
> > itself uses vma_area::start/end members for overlows tests internally
> > so I think even passing crazy data here won't crash the kernel itself.
> 
> Right, though besides just crashing the kernel, I'm trying to look at
> this from the perspective of a paranoid admin that doesn't even trust
> the root user. Is there some way this new interface could be used to
> provide the building blocks for gaining kernel execute control?
> (Imagine a system running with modules disabled, STRICT_DEVMEM
>  enabled, etc.)
> 

Well, I don't see a way to use it for gaining some control execution at the
moment but (!) kernel evolves and I fear one might forget about this new
interface when he will be doing some tossing over this mm_struct members.

I though about adding some kind of mark to kernel log on first usage of
this prctl codes (so if some crash happened for any reason, the log would
point someone used this interface).

> > What do you think?
> 
> This looks way better, yes. I have this feeling like these validations
> should be more centralized or tied to the mm code more directly to
> avoid drift, but I don't have any constructive suggestions
> unfortunately. Maybe other folks do?
> 

Yeah, good point, but on the other hands have these tests in one place
is easier to read I think.

> > +       switch (opt) {
> > +       case PR_SET_MM_START_CODE:
> > +       case PR_SET_MM_END_CODE:
> > +
> > +               vm_req_flags = VM_READ | VM_EXEC;
> > +               vm_bad_flags = VM_WRITE | VM_MAYSHARE;
> > +
> > +               if ((vma->vm_flags & vm_req_flags) != vm_req_flags ||
> > +                   (vma->vm_flags & vm_bad_flags))
> > +                       goto out;
> 
> Another random thought: given this very regular set of checks you're
> doing, perhaps the flags should be part of a data structure instead,
> just to reduce the size of this routine?
> 
> struct mm_flags {
>   int req_flags;
>   int bad_flags;
> };
> 
> struct mm_flags opt_flags[] = {
> ...
>  { VM_READ | VM_EXEC, VM_WRITE | VM_MAYSHARE }, /* PR_SET_MM_START_CODE */
>  { VM_READ | VM_EXEC, VM_WRITE | VM_MAYSHARE }, /* PR_SET_MM_END_CODE */
> ...
> 
> then do validation before the switch statement all in one place, and
> leave the switch for more programmatic checks?
> 
> -Kees
> 

Nod! I'll update, thanks!

	Cyrill