linux-kernel - Re: [PATCH v4 2/2] 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]

Date:   Thu, 11 May 2017 03:24:34 +0200
From:   "PaX Team" <pageexec@...email.hu>
To:     linux-kernel@...r.kernel.org, Kees Cook <keescook@...omium.org>
CC:     Kees Cook <keescook@...omium.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jann Horn <jannh@...gle.com>,
        Eric Biggers <ebiggers3@...il.com>,
        Christoph Hellwig <hch@...radead.org>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        James Bottomley <James.Bottomley@...senpartnership.com>,
        Elena Reshetova <elena.reshetova@...el.com>,
        Hans Liljestrand <ishkamiel@...il.com>,
        David Windsor <dwindsor@...il.com>,
        "x86@...nel.org" <x86@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S. Miller" <davem@...emloft.net>,
        Rik van Riel <riel@...hat.com>,
        linux-arch <linux-arch@...r.kernel.org>,
        kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v4 2/2] x86/refcount: Implement fast refcount overflow protection

On 9 May 2017 at 12:01, Kees Cook wrote:
> Various differences from PaX:
> - uses earlier value reset implementation in assembly
> - uses UD0 and refcount exception handler instead of new int vector
> - uses .text.unlikely instead of custom named text sections

all the above together result in bloating .text.unlikely and thus the
kernel image in general. the much bigger problem is that you introduced
a vulnerability because now refcount underflow bugs can not only trigger
a UAF but also a subsequent double-free since decrementing the saturation
value will not trigger any checks until 0 is reached a second time.

> - applied only to refcount_t, not atomic_t (single size, only overflow)

this description doesn't seem to be in sync with the code as the refcount
decrementing functions are also instrumented (and introduce the problem
mentioned above).

> - reorganized refcount error handler
> - uses "js" instead of "jo" to trap all negative results instead of
>   just under/overflow transitions

if you're describing differences to PaX in such detail you might as well
specify which version of PaX it is different from and credit the above idea
to me lest someone get the impression that it was yours.

> +static __always_inline __must_check bool refcount_inc_not_zero(refcount_t *r)
> +{
> +	int c;
> +
> +	c = atomic_read(&(r->refs));
> +	do {
> +		if (unlikely(c <= 0))
> +			break;
> +	} while (!atomic_try_cmpxchg(&(r->refs), &c, c + 1));
> +
> +	/* Did we start or finish in an undesirable state? */
> +	if (unlikely(c <= 0 || c + 1 < 0)) {

while -fno-strict-overflow should save you in linux it's still not
prudent programming to rely on signed overflow, especially in security
related checks. it's just too fragile and sets a bad example...