All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Hector Marco-Gisbert <hecmargi@upv.es>,
	Ismael Ripoll Ripoll <iripoll@upv.es>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Chen Gang <gang.chen.5i5j@gmail.com>,
	Michal Hocko <mhocko@suse.com>,
	Konstantin Khlebnikov <koct9i@gmail.com>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Andrey Ryabinin <aryabinin@virtuozzo.com>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	Linux-MM <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] mm: refuse wrapped vm_brk requests
Date: Wed, 13 Jul 2016 19:00:49 +0200	[thread overview]
Message-ID: <20160713170048.GA24553@redhat.com> (raw)
In-Reply-To: <CAGXu5j+oZ49K0omm-7yMsR_kFYD-DQcYG8f+urS+TumzFYXR_w@mail.gmail.com>

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.

WARNING: multiple messages have this Message-ID (diff)
From: Oleg Nesterov <oleg@redhat.com>
To: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Hector Marco-Gisbert <hecmargi@upv.es>,
	Ismael Ripoll Ripoll <iripoll@upv.es>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Chen Gang <gang.chen.5i5j@gmail.com>,
	Michal Hocko <mhocko@suse.com>,
	Konstantin Khlebnikov <koct9i@gmail.com>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Andrey Ryabinin <aryabinin@virtuozzo.com>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	Linux-MM <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] mm: refuse wrapped vm_brk requests
Date: Wed, 13 Jul 2016 19:00:49 +0200	[thread overview]
Message-ID: <20160713170048.GA24553@redhat.com> (raw)
In-Reply-To: <CAGXu5j+oZ49K0omm-7yMsR_kFYD-DQcYG8f+urS+TumzFYXR_w@mail.gmail.com>

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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2016-07-13 17:01 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-08 21:48 [PATCH 0/2] binfmt_elf: fix calculations for bss padding Kees Cook
2016-07-08 21:48 ` Kees Cook
2016-07-08 21:48 ` [PATCH 1/2] " Kees Cook
2016-07-08 21:48   ` Kees Cook
2016-07-12 22:32   ` Kees Cook
2016-07-12 22:32     ` Kees Cook
2016-07-08 21:48 ` [PATCH 2/2] mm: refuse wrapped vm_brk requests Kees Cook
2016-07-08 21:48   ` Kees Cook
2016-07-11 12:28   ` Oleg Nesterov
2016-07-11 12:28     ` Oleg Nesterov
2016-07-11 18:01     ` Kees Cook
2016-07-11 18:01       ` Kees Cook
2016-07-12 13:39       ` Oleg Nesterov
2016-07-12 13:39         ` Oleg Nesterov
2016-07-12 17:15         ` Kees Cook
2016-07-12 17:15           ` Kees Cook
2016-07-13 17:00           ` Oleg Nesterov [this message]
2016-07-13 17:00             ` Oleg Nesterov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160713170048.GA24553@redhat.com \
    --to=oleg@redhat.com \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=aryabinin@virtuozzo.com \
    --cc=gang.chen.5i5j@gmail.com \
    --cc=hecmargi@upv.es \
    --cc=iripoll@upv.es \
    --cc=keescook@chromium.org \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=koct9i@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.