Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

From: Peter Zijlstra <peterz@infradead.org>
To: Kees Cook <keescook@chromium.org>
Cc: "Reshetova, Elena" <elena.reshetova@intel.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>,
	"H. Peter Anvin" <h.peter.anvin@intel.com>,
	Will Deacon <will.deacon@arm.com>,
	David Windsor <dwindsor@gmail.com>,
	Hans Liljestrand <ishkamiel@gmail.com>,
	David Howells <dhowells@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"kernel-hardening@lists.openwall.com" 
	<kernel-hardening@lists.openwall.com>
Subject: Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION
Date: Mon, 6 Feb 2017 09:57:39 +0100	[thread overview]
Message-ID: <20170206085739.GH6515@twins.programming.kicks-ass.net> (raw)
In-Reply-To: <CAGXu5jLvpq6B80aN44JCOfHrov5LKX1_9r4aWb7LNM7pAW_SEQ@mail.gmail.com>

On Sun, Feb 05, 2017 at 03:33:36PM -0800, Kees Cook wrote:
> On Sun, Feb 5, 2017 at 7:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Fri, Feb 03, 2017 at 03:26:52PM -0800, Kees Cook wrote:
> >> This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
> >> CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
> >> conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
> >> is marked __much_check, we override few cases where the failure has
> >> already been handled but we want to explicitly report it.
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> ---
> >>  include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
> >>  lib/Kconfig.debug        |  2 ++
> >>  2 files changed, 24 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
> >> index 5b89cad62237..ef32910c7dd8 100644
> >> --- a/include/linux/refcount.h
> >> +++ b/include/linux/refcount.h
> >> @@ -43,10 +43,10 @@
> >>  #include <linux/spinlock.h>
> >>
> >>  #if CONFIG_DEBUG_REFCOUNT
> >> -#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
> >> +#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)
> >
> > OK, so that goes back to a full WARN() which will make the generated
> > code gigantic due to the whole printk() trainwreck :/
> 
> Hrm, perhaps we need three levels? WARN_ON, WARN, and BUG?

Did consider that, didn't really know if that made sense.

Like I wrote, ideally we'd end up using something like the x86 exception
table with a custom handler. Just no idea how to pull that off without
doing a full blown arch specific implementation, so I didn't go there
quite yet.

That way refcount_inc() would end up being inlined to something like:

        mov    0x148(%rdi),%eax
        jmp    2f
  1:    lock cmpxchg %edx,0x148(%rdi)
        je     4f
  2:    lea    -0x1(%rax),%ecx
        lea    0x1(%rax),%edx
        cmp    $0xfffffffd,%ecx
        jbe    1b
  3:    ud2
  4:

	_ASM_EXTABLE_HANDLE(3b, 4b, ex_handler_refcount_inc)

where:

bool ex_handler_refcount_inc(const struct exception_table_entry *fixup,
			     struct pt_regs *regs, int trapnr)
{
	regs->ip = ex_fixup_addr(fixup);

	if (!regs->ax)
		WARN(1, "refcount_t: increment on 0; use-after-free.\n");
	else
		WARN(1, "refcount_t: saturated; leaking memory.\n");

	return true;
}

and the handler is shared between all instances and can be as big and
fancy as we'd like.