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

[Peter Zijlstra <peterz@infradead.org>]

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.