Messages in this thread | | | Date | Mon, 11 Jul 2016 14:28:26 +0200 | From | Oleg Nesterov <> | Subject | 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.
| |