Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440524 > unrolled thread

Re: [PATCH 2/2] mm: refuse wrapped vm_brk requests

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] mm: refuse wrapped vm_brk requests Oleg Nesterov <oleg@redhat.com> - 2016-07-11 14:30 +0200
    Re: [PATCH 2/2] mm: refuse wrapped vm_brk requests Kees Cook <keescook@chromium.org> - 2016-07-11 20:10 +0200
      Re: [PATCH 2/2] mm: refuse wrapped vm_brk requests Oleg Nesterov <oleg@redhat.com> - 2016-07-12 15:40 +0200
        Re: [PATCH 2/2] mm: refuse wrapped vm_brk requests Kees Cook <keescook@chromium.org> - 2016-07-12 19:20 +0200
          Re: [PATCH 2/2] mm: refuse wrapped vm_brk requests Oleg Nesterov <oleg@redhat.com> - 2016-07-13 19:10 +0200

#1440524 — Re: [PATCH 2/2] mm: refuse wrapped vm_brk requests

I think both patches are fine, just a question.

On 07/08, Kees Cook wrote:
>
> -static int do_brk(unsigned long addr, unsigned long len)
> +static int do_brk(unsigned long addr, unsigned long request)
>  {
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma, *prev;
> -	unsigned long flags;
> +	unsigned long flags, len;
>  	struct rb_node **rb_link, *rb_parent;
>  	pgoff_t pgoff = addr >> PAGE_SHIFT;
>  	int error;
>  
> -	len = PAGE_ALIGN(len);
> +	len = PAGE_ALIGN(request);
> +	if (len < request)
> +		return -ENOMEM;

So iiuc "len < request" is only possible if len == 0, right?

>  	if (!len)
>  		return 0;

and thus this patch fixes the error code returned by do_brk() in case
of overflow, now it returns -ENOMEM rather than zero. Perhaps

	if (!len)
		return 0;
	len = PAGE_ALIGN(len);
	if (!len)
		return -ENOMEM;

would be more clear but this is subjective.

I am wondering if we should shift this overflow check to the caller(s).
Say, sys_brk() does find_vma_intersection(mm, oldbrk, newbrk+PAGE_SIZE)
before do_brk(), and in case of overflow find_vma_intersection() can
wrongly return NULL.

Then do_brk() will be called with len = -oldbrk, this can overflow or
not but in any case this doesn't look right too.

Or I am totally confused?

Oleg.

[toc] | [next] | [standalone]

#1440798

On Mon, Jul 11, 2016 at 8:28 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> I think both patches are fine, just a question.
>
> On 07/08, Kees Cook wrote:
>>
>> -static int do_brk(unsigned long addr, unsigned long len)
>> +static int do_brk(unsigned long addr, unsigned long request)
>>  {
>>       struct mm_struct *mm = current->mm;
>>       struct vm_area_struct *vma, *prev;
>> -     unsigned long flags;
>> +     unsigned long flags, len;
>>       struct rb_node **rb_link, *rb_parent;
>>       pgoff_t pgoff = addr >> PAGE_SHIFT;
>>       int error;
>>
>> -     len = PAGE_ALIGN(len);
>> +     len = PAGE_ALIGN(request);
>> +     if (len < request)
>> +             return -ENOMEM;
>
> So iiuc "len < request" is only possible if len == 0, right?

Oh, hrm, good point.

>
>>       if (!len)
>>               return 0;
>
> and thus this patch fixes the error code returned by do_brk() in case
> of overflow, now it returns -ENOMEM rather than zero. Perhaps
>
>         if (!len)
>                 return 0;
>         len = PAGE_ALIGN(len);
>         if (!len)
>                 return -ENOMEM;
>
> would be more clear but this is subjective.

I'm fine either way.

> I am wondering if we should shift this overflow check to the caller(s).
> Say, sys_brk() does find_vma_intersection(mm, oldbrk, newbrk+PAGE_SIZE)
> before do_brk(), and in case of overflow find_vma_intersection() can
> wrongly return NULL.
>
> Then do_brk() will be called with len = -oldbrk, this can overflow or
> not but in any case this doesn't look right too.
>
> Or I am totally confused?

I think the callers shouldn't request a negative value, sure, but
vm_brk should notice and refuse it.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1441359

On 07/11, Kees Cook wrote:
>
> On Mon, Jul 11, 2016 at 8:28 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > and thus this patch fixes the error code returned by do_brk() in case
> > of overflow, now it returns -ENOMEM rather than zero. Perhaps
> >
> >         if (!len)
> >                 return 0;
> >         len = PAGE_ALIGN(len);
> >         if (!len)
> >                 return -ENOMEM;
> >
> > would be more clear but this is subjective.
>
> I'm fine either way.

Me too, so feel free to ignore,

> > I am wondering if we should shift this overflow check to the caller(s).
> > Say, sys_brk() does find_vma_intersection(mm, oldbrk, newbrk+PAGE_SIZE)
> > before do_brk(), and in case of overflow find_vma_intersection() can
> > wrongly return NULL.
> >
> > Then do_brk() will be called with len = -oldbrk, this can overflow or
> > not but in any case this doesn't look right too.
> >
> > Or I am totally confused?
>
> I think the callers shouldn't request a negative value, sure, but
> vm_brk should notice and refuse it.

Not sure I understand...

I tried to say that, with or without this change, sys_brk() should check
for overflow too, otherwise it looks buggy.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1441606

On Tue, Jul 12, 2016 at 9:39 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 07/11, Kees Cook wrote:
>>
>> On Mon, Jul 11, 2016 at 8:28 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> > and thus this patch fixes the error code returned by do_brk() in case
>> > of overflow, now it returns -ENOMEM rather than zero. Perhaps
>> >
>> >         if (!len)
>> >                 return 0;
>> >         len = PAGE_ALIGN(len);
>> >         if (!len)
>> >                 return -ENOMEM;
>> >
>> > would be more clear but this is subjective.
>>
>> I'm fine either way.
>
> Me too, so feel free to ignore,
>
>> > I am wondering if we should shift this overflow check to the caller(s).
>> > Say, sys_brk() does find_vma_intersection(mm, oldbrk, newbrk+PAGE_SIZE)
>> > before do_brk(), and in case of overflow find_vma_intersection() can
>> > wrongly return NULL.
>> >
>> > Then do_brk() will be called with len = -oldbrk, this can overflow or
>> > not but in any case this doesn't look right too.
>> >
>> > Or I am totally confused?
>>
>> I think the callers shouldn't request a negative value, sure, but
>> vm_brk should notice and refuse it.
>
> Not sure I understand...
>
> I tried to say that, with or without this change, sys_brk() should check
> for overflow too, otherwise it looks buggy.

Hmm, it's not clear to me the right way to fix sys_brk(), but it looks
like my change to do_brk() would catch the problem?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1442641

On 07/12, Kees Cook wrote:
>
> On Tue, Jul 12, 2016 at 9:39 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > I tried to say that, with or without this change, sys_brk() should check
> > for overflow too, otherwise it looks buggy.
>
> Hmm, it's not clear to me the right way to fix sys_brk(), but it looks
> like my change to do_brk() would catch the problem?

How?

Once again, afaics nothing bad can happen, sys_brk() will silently fail,
just the code looks wrong anyway.

Suppose that newbrk == 0 due to overflow, then both

	if (find_vma_intersection(mm, oldbrk, newbrk+PAGE_SIZE))
		goto out;

and
	if (do_brk(oldbrk, newbrk-oldbrk) < 0)
		goto out;

look buggy.

find_vma_intersection(start_addr, end_addr) expects that start_addr < end_addr.
Again, we do not really care if it returns NULL or not, and newbrk == 0 just
means it will certainly return NULL if there is something above oldbrk. Just
looks buggy/confusing.

do_brk(0 - oldbrk) will fail and this is what we want. But not because
your change will catch the problem, PAGE_ALIGNE(-oldbrk) won't necessarily
overflow. However, -oldbrk > TASK_SIZE so get_unmapped_area() should fail.

Nevermind, this is almost off-topic, so let me repeat just in case that
both patches look good to me.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web