linux-kernel - Re: [PATCH 6/6] refcount: Check bad states with CHECK_DATA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jJBnjvY3qRmVo23bL6jJXfRB9+o=-pSg9UGEr4tfeBwzQ@mail.gmail.com>
Date:   Wed, 22 Mar 2017 12:30:39 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Rik van Riel <riel@...hat.com>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Olof Johansson <olof@...om.net>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH 6/6] refcount: Check bad states with CHECK_DATA_CORRUPTION

On Mon, Mar 6, 2017 at 11:09 AM, Kees Cook <keescook@...omium.org> wrote:
> This converts from WARN() to CHECK_DATA_CORRUPTION() (so that system
> builders can choose between WARN and BUG). Additionally moves refcount_t
> sanity-check conditionals into regular function flow.
>
> Now when built with CONFIG_BUG_ON_DATA_CORRUPTION, the LKDTM REFCOUNT_*
> tests correctly kill offending processes.

Any feedback on this change? I'd like to get this and the prior
patches into -next soon for more testing.

-Kees

>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
>  lib/refcount.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/lib/refcount.c b/lib/refcount.c
> index 1d33366189d1..54aff1e0582f 100644
> --- a/lib/refcount.c
> +++ b/lib/refcount.c
> @@ -37,6 +37,13 @@
>  #include <linux/refcount.h>
>  #include <linux/bug.h>
>
> +/*
> + * CHECK_DATA_CORRUPTION() is defined with __must_check, but we have a
> + * couple places where we want to report a condition that has already
> + * been checked, so this lets us cheat __must_check.
> + */
> +#define REFCOUNT_CHECK(cond, str) unlikely(CHECK_DATA_CORRUPTION(cond, str))
> +
>  bool refcount_add_not_zero(unsigned int i, refcount_t *r)
>  {
>         unsigned int old, new, val = atomic_read(&r->refs);
> @@ -58,7 +65,8 @@ bool refcount_add_not_zero(unsigned int i, refcount_t *r)
>                 val = old;
>         }
>
> -       WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
> +       REFCOUNT_CHECK(new == UINT_MAX,
> +                       "refcount_t: add saturated; leaking memory.\n");
>
>         return true;
>  }
> @@ -66,7 +74,8 @@ EXPORT_SYMBOL_GPL(refcount_add_not_zero);
>
>  void refcount_add(unsigned int i, refcount_t *r)
>  {
> -       WARN(!refcount_add_not_zero(i, r), "refcount_t: addition on 0; use-after-free.\n");
> +       REFCOUNT_CHECK(!refcount_add_not_zero(i, r),
> +                       "refcount_t: addition on 0; use-after-free.\n");
>  }
>  EXPORT_SYMBOL_GPL(refcount_add);
>
> @@ -97,7 +106,8 @@ bool refcount_inc_not_zero(refcount_t *r)
>                 val = old;
>         }
>
> -       WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
> +       REFCOUNT_CHECK(new == UINT_MAX,
> +                       "refcount_t: inc saturated; leaking memory.\n");
>
>         return true;
>  }
> @@ -111,7 +121,8 @@ EXPORT_SYMBOL_GPL(refcount_inc_not_zero);
>   */
>  void refcount_inc(refcount_t *r)
>  {
> -       WARN(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-after-free.\n");
> +       REFCOUNT_CHECK(!refcount_inc_not_zero(r),
> +                       "refcount_t: increment on 0; use-after-free.\n");
>  }
>  EXPORT_SYMBOL_GPL(refcount_inc);
>
> @@ -124,10 +135,9 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
>                         return false;
>
>                 new = val - i;
> -               if (new > val) {
> -                       WARN(new > val, "refcount_t: underflow; use-after-free.\n");
> +               if (REFCOUNT_CHECK(new > val,
> +                               "refcount_t: sub underflow; use-after-free.\n"))
>                         return false;
> -               }
>
>                 old = atomic_cmpxchg_release(&r->refs, val, new);
>                 if (old == val)
> @@ -164,7 +174,8 @@ EXPORT_SYMBOL_GPL(refcount_dec_and_test);
>
>  void refcount_dec(refcount_t *r)
>  {
> -       WARN(refcount_dec_and_test(r), "refcount_t: decrement hit 0; leaking memory.\n");
> +       REFCOUNT_CHECK(refcount_dec_and_test(r),
> +                       "refcount_t: decrement hit 0; leaking memory.\n");
>  }
>  EXPORT_SYMBOL_GPL(refcount_dec);
>
> @@ -203,10 +214,9 @@ bool refcount_dec_not_one(refcount_t *r)
>                         return false;
>
>                 new = val - 1;
> -               if (new > val) {
> -                       WARN(new > val, "refcount_t: underflow; use-after-free.\n");
> +               if (REFCOUNT_CHECK(new > val,
> +                               "refcount_t: dec underflow; use-after-free.\n"))
>                         return true;
> -               }
>
>                 old = atomic_cmpxchg_release(&r->refs, val, new);
>                 if (old == val)
> @@ -264,4 +274,3 @@ bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
>         return true;
>  }
>  EXPORT_SYMBOL_GPL(refcount_dec_and_lock);
> -
> --
> 2.7.4
>



-- 
Kees Cook
Pixel Security