Arithmetic overflow in may_expand_vm()

Arithmetic overflow in may_expand_vm()

[Earl Chew]

This code currently reads:

> int may_expand_vm(struct mm_struct *mm, unsigned long npages)
> {
>         unsigned long cur = mm->total_vm;       /* pages */
>         unsigned long lim;
> 
>         lim = current->signal->rlim[RLIMIT_AS].rlim_cur >> PAGE_SHIFT;
> 
>         if (cur + npages > lim)
>                 return 0;
>         return 1;
> }

If npages is stupendously large, the failure predicate may
return a false negative due to (cur + npages) overflowing and
wrapping.

I think it's more robustly written as:

          if (npages > lim - cur)
                  return 0;

Re: Arithmetic overflow in may_expand_vm()

[Johannes Weiner]

Hi,

On Thu, Oct 15, 2009 at 10:24:51AM -0700, Earl Chew wrote:
> This code currently reads:
> 
> >int may_expand_vm(struct mm_struct *mm, unsigned long npages)
> >{
> >        unsigned long cur = mm->total_vm;       /* pages */
> >        unsigned long lim;
> >
> >        lim = current->signal->rlim[RLIMIT_AS].rlim_cur >> PAGE_SHIFT;
> >
> >        if (cur + npages > lim)
> >                return 0;
> >        return 1;
> >}
> 
> If npages is stupendously large, the failure predicate may
> return a false negative due to (cur + npages) overflowing and
> wrapping.

Can this really happen?

npages always originates in a value of byte granularity, giving a
theoretical maximum of ~0UL >> PAGE_SHIFT (checking for more than the
number of addressable bytes just makes no sense).

And mm->total_vm is always PAGE_SIZE times smaller than total user
address space (which in turn is always less than ~0UL).

So I can not see this overflow being possible with PAGE_SHIFT > 0.

Re: Arithmetic overflow in may_expand_vm()

[Earl Chew]

Johannes Weiner wrote:
>> If npages is stupendously large, the failure predicate may
>> return a false negative due to (cur + npages) overflowing and
>> wrapping.
> 
> Can this really happen?
> 
> npages always originates in a value of byte granularity, giving a
> theoretical maximum of ~0UL >> PAGE_SHIFT (checking for more than the
> number of addressable bytes just makes no sense).

I think you're saying that there are no (external facing)
interfaces that ask for pages -- they always ask for octets.

You may well be right. I don't know the kernel code base well
enough to say for sure one way or another.

> And mm->total_vm is always PAGE_SIZE times smaller than total user
> address space (which in turn is always less than ~0UL).
> 
> So I can not see this overflow being possible with PAGE_SHIFT > 0.

A very reasonable argument to be sure.

I can think of two counter-arguments:

a. The fewer assumptions made by may_expand_vm() (or any other
    function for that matter) about its callers, the more robust
    the function, and the more resilient the system.

    I think it would be good practice for may_expand_vm() to
    do the right thing for all possible input values. Especially
    in this case where the cost of doing the right thing is either
    very small or zero.

b. There are other examples in the code base that use the more
    robust approach. For instance see kernel/filemap.c:

         unsigned long limit =
            current->signal->rlim[RLIMIT_FSIZE].rlim_cur;

            ... snip ...

                 if (limit != RLIM_INFINITY) {
                         if (*pos >= limit) {
                                 send_sig(SIGXFSZ, current, 0);
                                 return -EFBIG;
                         }
                         if (*count > limit - (typeof(limit))*pos) {
                                 *count = limit - (typeof(limit))*pos;
                         }
                 }


Earl

Re: Arithmetic overflow in may_expand_vm()

[Johannes Weiner]

On Mon, Oct 19, 2009 at 07:43:01AM -0700, Earl Chew wrote:
> Johannes Weiner wrote:
> >>If npages is stupendously large, the failure predicate may
> >>return a false negative due to (cur + npages) overflowing and
> >>wrapping.
> >
> >Can this really happen?
> >
> >npages always originates in a value of byte granularity, giving a
> >theoretical maximum of ~0UL >> PAGE_SHIFT (checking for more than the
> >number of addressable bytes just makes no sense).
> 
> I think you're saying that there are no (external facing)
> interfaces that ask for pages -- they always ask for octets.

Yes.

> You may well be right. I don't know the kernel code base well
> enough to say for sure one way or another.
> 
> >And mm->total_vm is always PAGE_SIZE times smaller than total user
> >address space (which in turn is always less than ~0UL).
> >
> >So I can not see this overflow being possible with PAGE_SHIFT > 0.
> 
> A very reasonable argument to be sure.
> 
> I can think of two counter-arguments:
> 
> a. The fewer assumptions made by may_expand_vm() (or any other
>    function for that matter) about its callers, the more robust
>    the function, and the more resilient the system.
> 
>    I think it would be good practice for may_expand_vm() to
>    do the right thing for all possible input values. Especially
>    in this case where the cost of doing the right thing is either
>    very small or zero.

But implicitely defending against something we do not expect to happen
is misleading.

If you expect overflows to be possible (when validating user input
e.g.), check for them.  But please don't make the code suggest they
could appear in any sane condition when they in fact do not.

I think that has better chances of making semantics and misuse more
obvious.

> b. There are other examples in the code base that use the more
>    robust approach. For instance see kernel/filemap.c:
> 
>         unsigned long limit =
>            current->signal->rlim[RLIMIT_FSIZE].rlim_cur;
> 
>            ... snip ...
> 
>                 if (limit != RLIM_INFINITY) {
>                         if (*pos >= limit) {
>                                 send_sig(SIGXFSZ, current, 0);
>                                 return -EFBIG;
>                         }
>                         if (*count > limit - (typeof(limit))*pos) {
>                                 *count = limit - (typeof(limit))*pos;
>                         }
>                 }

And it is misleading.  That function (generic_write_checks) can not
cope with overflows anyway and callsites should have sorted that out
long before.  But some of those checks suggest otherwise.