linux-kernel - Re: [PATCH v7 3/3] x86/refcount: Implement fast refcount overflow protection

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: <20170724090710.o66r5r2pwxgwor7t@gmail.com>
Date:   Mon, 24 Jul 2017 11:07:10 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Christoph Hellwig <hch@...radead.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jann Horn <jannh@...gle.com>,
        Eric Biggers <ebiggers3@...il.com>,
        Elena Reshetova <elena.reshetova@...el.com>,
        Hans Liljestrand <ishkamiel@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        "Serge E. Hallyn" <serge@...lyn.com>, arozansk@...hat.com,
        Davidlohr Bueso <dave@...olabs.net>,
        Manfred Spraul <manfred@...orfullife.com>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        James Bottomley <James.Bottomley@...senpartnership.com>,
        "x86@...nel.org" <x86@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        "David S. Miller" <davem@...emloft.net>,
        Rik van Riel <riel@...hat.com>, linux-kernel@...r.kernel.org,
        linux-arch <linux-arch@...r.kernel.org>,
        "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v7 3/3] x86/refcount: Implement fast refcount overflow
 protection


* Kees Cook <keescook@...omium.org> wrote:

> +config ARCH_HAS_REFCOUNT
> +     bool
> +     help
> +       An architecture selects this when it has implemented refcount_t
> +       using primitizes that provide a faster runtime at the expense
> +       of some full refcount state checks. The refcount overflow condition,
> +       however, must be retained. Catching overflows is the primary
> +       security concern for protecting against bugs in reference counts.

s/primitizes/primitives

also, the 'faster runtime' and the whole explanation reads a bit weird to me,
how about something like:

       An architecture selects this when it has implemented refcount_t
       using open coded assembly primitives that provide an optimized
       refcount_t implementation, possibly at the expense of some full
       refcount state checks of CONFIG_REFCOUNT_FULL=y.

       The refcount overflow check behavior, however, must be retained.
       Catching overflows is the primary security concern for protecting
       against bugs in reference counts.

> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -55,6 +55,7 @@ config X86
>  	select ARCH_HAS_KCOV			if X86_64
>  	select ARCH_HAS_MMIO_FLUSH
>  	select ARCH_HAS_PMEM_API		if X86_64
> +	select ARCH_HAS_REFCOUNT
>  	select ARCH_HAS_UACCESS_FLUSHCACHE	if X86_64
>  	select ARCH_HAS_SET_MEMORY
>  	select ARCH_HAS_SG_CHAIN

Just wonderin, how was the 32-bit kernel tested?

> +/*
> + * Body of refcount error handling: in .text.unlikely, saved into CX the
> + * address of the refcount that has entered a bad state, and trigger an
> + * exception. Fixup address is back in regular execution flow in .text.

I had to read this 4 times to parse it (and even now I'm unsure whether I parsed 
it correctly) - could this explanation be transformed to simpler, more 
straightforward English?

> + */
> +#define _REFCOUNT_EXCEPTION				\
> +	".pushsection .text.unlikely\n"			\
> +	"111:\tlea %[counter], %%" _ASM_CX "\n"		\
> +	"112:\t" ASM_UD0 "\n"				\
> +	ASM_UNREACHABLE					\
> +	".popsection\n"					\
> +	"113:\n"					\
> +	_ASM_EXTABLE_REFCOUNT(112b, 113b)

Would it be technically possible to use named labels instead of these random 
numbered labels?

> +	/*
> +	 * This function has been called because either a negative refcount
> +	 * value was seen by any of the refcount functions, or a zero
> +	 * refcount value was seen by refcount_dec().
> +	 *
> +	 * If we crossed from INT_MAX to INT_MIN, the OF flag (result
> +	 * wrapped around) will be set. Additionally, seeing the refcount
> +	 * reach 0 will set the ZF flag. In each of these cases we want a
> +	 * report, since it's a boundary condition.

Small nit: 'ZF' stands for 'zero flag' - so we should either write 'zero flag' or 
'ZF' - 'ZF flag' is kind of redundant.

> +#else
> +static inline void refcount_error_report(struct pt_regs *regs,
> +					 const char *msg) { }

By now you should know that for x86 code you should not break lines in such an 
ugly fashion, right? :-)

Thanks,

	Ingo