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

Groups > linux.kernel > #1307763 > unrolled thread

[RFC] kernel/panic: place an upper limit on number of oopses

Back to article view | Back to linux.kernel

Contents

  [RFC] kernel/panic: place an upper limit on number of oopses Jann Horn <jann@thejh.net> - 2016-01-12 20:30 +0100
    Re: [RFC] kernel/panic: place an upper limit on number of oopses Daniel Axtens <dja@axtens.net> - 2016-01-13 00:40 +0100
      Re: [RFC] kernel/panic: place an upper limit on number of oopses Jann Horn <jann@thejh.net> - 2016-01-13 01:00 +0100
      Re: [RFC] kernel/panic: place an upper limit on number of oopses Solar Designer <solar@openwall.com> - 2016-01-13 01:30 +0100
        Re: [RFC] kernel/panic: place an upper limit on number of oopses Daniel Axtens <dja@axtens.net> - 2016-01-13 01:40 +0100
        Re: [RFC] kernel/panic: place an upper limit on number of oopses Jann Horn <jann@thejh.net> - 2016-01-13 19:10 +0100

#1307763 — [RFC] kernel/panic: place an upper limit on number of oopses

To prevent an attacker from turning a mostly harmless oops into an
exploitable issue using a refcounter wraparound caused by repeated
oopsing, limit the number of oopses.

I have not experimentally verified whether the attack I describe
in the comment works, but I don't see why it wouldn't.
(f_count increments through fget() use atomic_long_inc_not_zero(),
but get_file() just does a normal increment and is e.g.
used by dup_fd().)

This approach is strictly inferior to PAX_REFCOUNT, but as long
as that's not upstreamed and turned on by default, it might make
sense to at least use this patch.

Opinions?

Signed-off-by: Jann Horn <jann@thejh.net>
---
 kernel/panic.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/kernel/panic.c b/kernel/panic.c
index 4b150bc..27a480d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -422,9 +422,37 @@ void print_oops_end_marker(void)
  */
 void oops_exit(void)
 {
+	static atomic_t oops_counter = ATOMIC_INIT(0);
+
 	do_oops_enter_exit();
 	print_oops_end_marker();
 	kmsg_dump(KMSG_DUMP_OOPS);
+
+	/*
+	 * Every time the system oopses, if the oops happens while a
+	 * reference to an object was held (e.g. in a VFS function),
+	 * the reference leaks. If the oops doesn't also leak memory,
+	 * repeated oopsing can cause the reference counter to wrap
+	 * around - in particular, on 32bit systems, f_count in
+	 * struct file is only 32 bits long and can realistically
+	 * wrap around.
+	 * This means that an oops, even if it's just caused by an
+	 * unexploitable-looking NULL pointer dereference or so,
+	 * could maybe be turned into a use-after-free through a
+	 * counter overincrement, and a use-after-free might be
+	 * exploitable.
+	 * To reduce the probability that this happens, place an
+	 * upper bound on how often the kernel may oops - after this
+	 * limit is reached, just panic.
+	 * The constant used as limit should be low enough to
+	 * mitigate this kind of exploitation attempt, but high
+	 * enough to avoid unnecessary panics.
+	 */
+	if (atomic_inc_return(&oops_counter) >= 0x100000 &&
+			panic_on_oops == 0) {
+		pr_emerg("oopsed too often, setting panic_on_oops=1\n");
+		panic_on_oops = 1;
+	}
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
-- 
2.1.4

[toc] | [next] | [standalone]

#1307919

[Multipart message — attachments visible in raw view] — view raw

Jann Horn <jann@thejh.net> writes:

> To prevent an attacker from turning a mostly harmless oops into an
> exploitable issue using a refcounter wraparound caused by repeated
> oopsing, limit the number of oopses.
>
> I have not experimentally verified whether the attack I describe
> in the comment works, but I don't see why it wouldn't.
> (f_count increments through fget() use atomic_long_inc_not_zero(),
> but get_file() just does a normal increment and is e.g.
> used by dup_fd().)
>
> This approach is strictly inferior to PAX_REFCOUNT, but as long
> as that's not upstreamed and turned on by default, it might make
> sense to at least use this patch.
>
> Opinions?

I'm torn between making the limit configurable and not adding to the
massive proliferation of config options.

Other comments below.
>
> Signed-off-by: Jann Horn <jann@thejh.net>
> ---
>  kernel/panic.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 4b150bc..27a480d 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -422,9 +422,37 @@ void print_oops_end_marker(void)
>   */
>  void oops_exit(void)
>  {
> +	static atomic_t oops_counter = ATOMIC_INIT(0);
> +
>  	do_oops_enter_exit();
>  	print_oops_end_marker();
>  	kmsg_dump(KMSG_DUMP_OOPS);
> +
> +	/*
> +	 * Every time the system oopses, if the oops happens while a
> +	 * reference to an object was held (e.g. in a VFS function),
> +	 * the reference leaks. If the oops doesn't also leak memory,
> +	 * repeated oopsing can cause the reference counter to wrap
> +	 * around - in particular, on 32bit systems, f_count in
> +	 * struct file is only 32 bits long and can realistically
> +	 * wrap around.
> +	 * This means that an oops, even if it's just caused by an
> +	 * unexploitable-looking NULL pointer dereference or so,
> +	 * could maybe be turned into a use-after-free through a
> +	 * counter overincrement, and a use-after-free might be
> +	 * exploitable.
> +	 * To reduce the probability that this happens, place an
> +	 * upper bound on how often the kernel may oops - after this
> +	 * limit is reached, just panic.
> +	 * The constant used as limit should be low enough to
> +	 * mitigate this kind of exploitation attempt, but high
> +	 * enough to avoid unnecessary panics.
> +	 */
> +	if (atomic_inc_return(&oops_counter) >= 0x100000 &&
> +			panic_on_oops == 0) {
Do you need to check panic_on_oops? If it was 1 you'd already have
paniced, right?
> +		pr_emerg("oopsed too often, setting panic_on_oops=1\n");
> +		panic_on_oops = 1;
Would it be easier to just panic here, rather than wait for another oops?
> +	}
>  }
>  
>  #ifdef WANT_WARN_ON_SLOWPATH

Regards,
Daniel
> -- 
> 2.1.4

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

#1307928

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jan 13, 2016 at 10:34:39AM +1100, Daniel Axtens wrote:
> Jann Horn <jann@thejh.net> writes:
> > +	 * limit is reached, just panic.
> > +	 * The constant used as limit should be low enough to
> > +	 * mitigate this kind of exploitation attempt, but high
> > +	 * enough to avoid unnecessary panics.
> > +	 */
> > +	if (atomic_inc_return(&oops_counter) >= 0x100000 &&
> > +			panic_on_oops == 0) {
> Do you need to check panic_on_oops? If it was 1 you'd already have
> paniced, right?
[...]
> > +		pr_emerg("oopsed too often, setting panic_on_oops=1\n");
> > +		panic_on_oops = 1;
> Would it be easier to just panic here, rather than wait for another oops?

Ah, yes. So the code would be just this, apart from the definition of
oops_counter:

        if (atomic_inc_return(&oops_counter) >= 0x100000)
                panic("oopsed too often\n");

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

#1307936

Jann Horn <jann@thejh.net> wrote:
> To prevent an attacker from turning a mostly harmless oops into an
> exploitable issue using a refcounter wraparound caused by repeated
> oopsing, limit the number of oopses.

This may also reduce the likelihood of successful exploitation of some
other vulnerabilities involving memory corruption, where an unsuccessful
attempt may inadvertently trigger an Oops.  The attacker would then need
to succeed in fewer than the maximum allowed number of Oops'es.  Jann's
currently proposed default of 0x100000 is too high to make a difference
in that respect, but people may set it differently.

On Wed, Jan 13, 2016 at 10:34:39AM +1100, Daniel Axtens wrote:
> I'm torn between making the limit configurable and not adding to the
> massive proliferation of config options.

What about reusing panic_on_oops for the configurable limit?  The
currently supported values of 0 and 1 would retain their meaning,
2 would panic after 2nd Oops, and so on.

There's overlap with grsecurity's banning of users on Oops, but I think
it makes sense to have both the trivial change proposed by Jann (perhaps
with the reuse of panic_on_oops for configuration) and grsecurity-style
banning (maybe with a low configurable limit, rather than always on
first Oops).

Alexander

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

#1307941

Solar Designer <solar@openwall.com> writes:

> Jann Horn <jann@thejh.net> wrote:
>> To prevent an attacker from turning a mostly harmless oops into an
>> exploitable issue using a refcounter wraparound caused by repeated
>> oopsing, limit the number of oopses.
>
> This may also reduce the likelihood of successful exploitation of some
> other vulnerabilities involving memory corruption, where an unsuccessful
> attempt may inadvertently trigger an Oops.  The attacker would then need
> to succeed in fewer than the maximum allowed number of Oops'es.  Jann's
> currently proposed default of 0x100000 is too high to make a difference
> in that respect, but people may set it differently.
>
> On Wed, Jan 13, 2016 at 10:34:39AM +1100, Daniel Axtens wrote:
>> I'm torn between making the limit configurable and not adding to the
>> massive proliferation of config options.
>
> What about reusing panic_on_oops for the configurable limit?  The
> currently supported values of 0 and 1 would retain their meaning,
> 2 would panic after 2nd Oops, and so on.

I thought about this, then I looked at where panic_on_oops was used and
I thought it would be a pretty invasive change. I'm also nervous about
changing the semantics of panic_on_oops under people...

>
> There's overlap with grsecurity's banning of users on Oops, but I think
> it makes sense to have both the trivial change proposed by Jann (perhaps
> with the reuse of panic_on_oops for configuration) and grsecurity-style
> banning (maybe with a low configurable limit, rather than always on
> first Oops).
>
> Alexander

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

#1308700

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jan 13, 2016 at 03:20:43AM +0300, Solar Designer wrote:
> Jann Horn <jann@thejh.net> wrote:
> > To prevent an attacker from turning a mostly harmless oops into an
> > exploitable issue using a refcounter wraparound caused by repeated
> > oopsing, limit the number of oopses.
> 
> This may also reduce the likelihood of successful exploitation of some
> other vulnerabilities involving memory corruption, where an unsuccessful
> attempt may inadvertently trigger an Oops.  The attacker would then need
> to succeed in fewer than the maximum allowed number of Oops'es.  Jann's
> currently proposed default of 0x100000 is too high to make a difference
> in that respect, but people may set it differently.

I chose such a high value to increase the likelyhood that this gets
included in the kernel by default. Lower values would mitigate more
attacks, but I'm not sure whether they'd be acceptable for everyone.


> On Wed, Jan 13, 2016 at 10:34:39AM +1100, Daniel Axtens wrote:
> > I'm torn between making the limit configurable and not adding to the
> > massive proliferation of config options.
> 
> What about reusing panic_on_oops for the configurable limit?  The
> currently supported values of 0 and 1 would retain their meaning,
> 2 would panic after 2nd Oops, and so on.
> 
> There's overlap with grsecurity's banning of users on Oops, but I think
> it makes sense to have both the trivial change proposed by Jann (perhaps
> with the reuse of panic_on_oops for configuration) and grsecurity-style
> banning (maybe with a low configurable limit, rather than always on
> first Oops).

One edgecase here is that, afaik, grsecurity-style banning isn't very
effective in combination with the subuid mechanism (implemented in
userland, using the newuidmap setuid helper and /etc/subuid) because
it allows every user to control 2^16 kuids (not just inside namespaces,
but also indirectly in the init namespace).
This probably doesn't affect many people though: Debian and Ubuntu ship
newuidmap in a separate package "uidmap" that isn't installed by
default and is only installed by a few people (0.18% on Debian
according to popcon, those probably need it for unprivileged LXC or
so?). Arch ships with newuidmap installed, but without /etc/subuid.
I don't know what other distros do.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web