Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 10 Nov 2016 15:07:37 -0800
From: Kees Cook <keescook@...omium.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Elena Reshetova <elena.reshetova@...el.com>, 
	"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, Arnd Bergmann <arnd@...db.de>, 
	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, 
	"H. Peter Anvin" <h.peter.anvin@...el.com>, Will Deacon <will.deacon@....com>, 
	Hans Liljestrand <ishkamiel@...il.com>, David Windsor <dwindsor@...il.com>
Subject: Re: [RFC v4 PATCH 12/13] x86: implementation for HARDENED_ATOMIC

On Thu, Nov 10, 2016 at 2:50 PM, Peter Zijlstra <peterz@...radead.org> wrote:
> On Thu, Nov 10, 2016 at 09:40:46PM +0100, Peter Zijlstra wrote:
>> On Thu, Nov 10, 2016 at 10:24:47PM +0200, Elena Reshetova wrote:
>> >  static __always_inline void atomic_add(int i, atomic_t *v)
>> >  {
>> > +   asm volatile(LOCK_PREFIX "addl %1,%0\n"
>> > +
>> > +#ifdef CONFIG_HARDENED_ATOMIC
>> > +                "jno 0f\n"
>> > +                LOCK_PREFIX "subl %1,%0\n"
>> > +                "int $4\n0:\n"
>> > +                _ASM_EXTABLE(0b, 0b)
>>
>>
>> This is unreadable gunk.
>
> Worse, this is fundamentally racy and therefore not a proper atomic at
> all.

The race was discussed earlier as well (and some of that should
already have been covered in the commit message), but basically this
is a race in the overflow protection, so it's not operationally a
problem. The process that caused the overflow still gets killed, and
if the system isn't already set up to panic on oops, this becomes a
resource leak instead of an exploitable condition.

> The only way to do this is a cmpxchg loop and not issue the result on
> overflow. Of course, that would completely suck performance wise, but
> having a non atomic atomic blows more.

There was some work to examine this performance impact, and it didn't
look terrible, but this race-on-overflow isn't viewed as a meaningful
risk in the refcount situation.

-Kees

-- 
Kees Cook
Nexus Security

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.