| Message ID | 1468014494-25291-3-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/mm/mmap.c b/mm/mmap.c index de2c1769cc68..1874ee0e1266 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -2625,16 +2625,18 @@ static inline void verify_mm_writelocked(struct mm_struct *mm) * anonymous maps. eventually we may be able to do some * brk-specific accounting here. */ -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; if (!len) return 0;
The vm_brk() alignment calculations should refuse to overflow. The ELF loader depending on this, but it has been fixed now. No other unsafe callers have been found. Reported-by: Hector Marco-Gisbert <hecmargi@upv.es> Signed-off-by: Kees Cook <keescook@chromium.org> --- mm/mmap.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)