On Tue, Jul 12, 2016 at 9:39 AM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > On 07/11, Kees Cook wrote: >> >> On Mon, Jul 11, 2016 at 8:28 AM, Oleg Nesterov <oleg@xxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
