Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size

[pageexec () freemail hu]

On 10 Sep 2010 at 1:59, Roland McGrath wrote:

> > > + if (unlikely(stack_top < mmap_min_addr) ||
> >
> > this could arguably elicit some warning, or even better, prevent the
> > sysctl from setting mmap_min_addr too high in the first place.
>
> This code has local checks to ensure that things don't fail worse later.
> Those are good to have regardless.  If you'd like to add some constraints
> on setting mmap_min_addr, that's certainly fine too.  But it's no reason
> not to have this simple and clear check here.

it's somewhat confusing as it does not immediately relate to the problem
you're addressing. let's take a step back and see what issues we have here:

1. the would-be stack vma shifts down so much that it becomes completely
   invalid (due to the int wraparound on its start address).

2. the would be stack vma shifts down to violate mmap_min_addr.

it's obvious that before ensuring 2, 1 must pass. the check for 1 does not
at all involve mmap_min_addr, whereas both of your checks do. even worse, the
check for 2 doesn't depend on the relation between stack_top and mmap_min_addr
per se (userland has no influence over them), so as i said, it's all confusing.

> > or alternatively, just check for
> >
> >     mmap_min_addr > stack_top - (vma->vm_end - vma->vm_start)
>
> IMHO that is far less clear as to what the intent of the check is than what
> I wrote.

why is that? it's obvious that vm_end-vm_size=vm_size and the check simply
ensures that problem 2 above doesn't occur whereas in neither of your checks
can one see the clear intent for ensuring 2. your 2nd check basically ensures
that vm_size fits in the usable address space bounded by [mmap_min_addr, stack_top]
but it doesn't immediately tell the user why that size check is useful (especially
since we're not concerned with sizes per se, but addresses, it's just a corollary
in this particular case that a size check can replace an address check, but that
doesn't mean it'll help any future reader see the original intent). so while
the math is ok in your checks (modulo the = case), it's a kind of premature
optimization that hides the problems you're addressing.

>  It's especially subtle that this check allows overflow
> and then you check for that later,

no, actually that particular check was for addressing one particular
problem (to show you how to formulate it that makes intent clear), as a
train of thought if you like. the final proposed version (which has been
in PaX) has the int overflow check first and then the mmap_min_addr check.

> rather than the formulation I gave
> where no overflow is possible and it's immediately obvious why. 

you're only focused on the math and let the human intent slip by, that's
the issue i have with your checks (and you didn't get the = corner case
right btw, it should be allowed, nor refused if you want to be pedantic).

really, in security clarity of intent and its manifestation in code takes
precedence over optimization, let the compiler do the latter job.

> > which looks even better if done in shift_arg_pages as i've done it in PaX:
>
> My change to setup_arg_pages is consistent with the existing
> CONFIG_STACK_GROWSUP code.

the GROWSUP code doesn't use unlikely (very confusing to see it in this
context since you're not addressing any hot path/performance issues), nor
does it care about mmap_min_addr (whereas both of your checks do which
depending on the generated asm, may even be racy when mmap_min_addr changes)
nor does it check for any int overflows. about the only similarity i can
see is its computation of 'vm_size' and an associated size check (properly
commented unlike your code) but it's normal there, whereas the problems you're
addressing are fundamentally vma address related, not vma size related.

> IMHO simple fixes should go in first
> and other restructuring of the code can be done later.

yours is not a simple fix as it doesn't make it clear what two problems
you're trying to address with your checks.

> > > +         return -ENOMEM;
> >
> > i'd say EFAULT is more consistent, ENOMEM is used for cases when the kernel
> > couldn't allocate physical memory for something, EFAULT is when there's some
> > issue with the address space itself (based on the reaction to find_vma or
> > expand_stack failures).
>
> I disagree.

well, in that case the EFAULT return codes in this function need some
cleanup too, you know, in the name of consistency ;).

> > also what about the BUG_ON in shift_arg_pages? it could go now, right?
>
> It is now impossible by the logic of the arithmetic, yes.  But it is
> another local sanity check asserting the assumptions of the code in that
> function.  So there is no reason to 

there is no reason for it to be a BUG_ON (never was in fact), the kernel
can easily recover from the detected condition.

cheers,

 PaX Team

PS: i thought it was common courtesy and customary to credit people in the
commit who found/reported/fixed problems.