[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2236FBA76BA1254E88B949DDB74E612B41C32F34@IRSMSX102.ger.corp.intel.com>
Date: Fri, 30 Dec 2016 13:17:08 +0000
From: "Reshetova, Elena" <elena.reshetova@...el.com>
To: Eric Biggers <ebiggers3@...il.com>, "kernel-hardening@...ts.openwall.com"
<kernel-hardening@...ts.openwall.com>
CC: "keescook@...omium.org" <keescook@...omium.org>, "arnd@...db.de"
<arnd@...db.de>, "tglx@...utronix.de" <tglx@...utronix.de>,
"mingo@...hat.com" <mingo@...hat.com>, "Anvin, H Peter"
<h.peter.anvin@...el.com>, "peterz@...radead.org" <peterz@...radead.org>,
"will.deacon@....com" <will.deacon@....com>, "dwindsor@...il.com"
<dwindsor@...il.com>, "gregkh@...uxfoundation.org"
<gregkh@...uxfoundation.org>, "ishkamiel@...il.com" <ishkamiel@...il.com>
Subject: RE: [RFC PATCH 06/19] Provide refcount_t, an
atomic_t like primitive built just for refcounting.
> On Thu, Dec 29, 2016 at 08:55:58AM +0200, Elena Reshetova wrote:
> > +
> > +static inline __must_check
> > +bool refcount_add_not_zero(unsigned int i, refcount_t *r)
> > +{
> > + unsigned int old, new, val = atomic_read(&r->refs);
> > +
> > + for (;;) {
> > + if (!val)
> > + return false;
> > +
> > + if (unlikely(val == UINT_MAX))
> > + return true;
> > +
> > + new = val + i;
> > + if (new < val)
> > + new = UINT_MAX;
> > + old = atomic_cmpxchg_relaxed(&r->refs, val, new);
> > + if (old == val)
> > + break;
> > +
> > + val = old;
> > + }
> > +
> > + WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
> > +
> > + return true;
> > +}
> > +
> > +/*
> > + * Similar to atomic_inc_not_zero(), will saturate at UINT_MAX and WARN.
> > + *
> > + * Provides no memory ordering, it is assumed the caller has guaranteed the
> > + * object memory to be stable (RCU, etc.). It does provide a control dependency
> > + * and thereby orders future stores. See the comment on top.
> > + */
> > +static inline __must_check
> > +bool refcount_inc_not_zero(refcount_t *r)
> > +{
> > + return refcount_add_not_zero(1, r);
> > +}
> > +
> > +/*
> > + * Similar to atomic_inc(), will saturate at UINT_MAX and WARN.
> > + *
> > + * Provides no memory ordering, it is assumed the caller already has a
> > + * reference on the object, will WARN when this is not so.
> > + */
> > +static inline void refcount_inc(refcount_t *r)
> > +{
> > + WARN(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-after-
> free.\n");
> > +}
> > +
>
> ... and refcount_inc() compiles to over 100 bytes of instructions on x86_64.
> This is the wrong approach. We need a low-overhead solution, otherwise no one
> will turn on refcount protection and the feature will be useless.
>
> What exactly is wrong with the current solution in PAX/grsecurity? Looking at
> the x86 version they have atomic_inc() do 'lock incl' like usual, then use 'jo'
> to, if the counter overflowed, jump to *out-of-line* error handling code, in a
> separate section of the kernel image. Then it raises a software interrupt, and
> the interrupt handler sets the overflowed counter to INT_MAX and does the
> needed
> logging and signal raising.
>
> That approach seems very efficient. It seems the only overhead when someone
> isn't actually exploiting a refcount bug is the 'jo' instruction, with the
> branch not taken. There aren't even any other in-line instructions to waste
> icache space.
> I do see they used to use a slightly different approach that did a decrement
> instead of setting the counter to INT_MAX. And that was clearly racy because
> two concurrent increments could circumvent the overflow protection. But AFAICS
> their current solution is not racy in any meaningful way, since the setting to
> INT_MAX means an overflow will be detected again on the next increment, even if
> there were some concurrent increments in the mean time. (And if by some stretch
> of the imagination, it was possible to execute *another* INT_MAX increments
> before the fixup code had a chance to run, the correct solution would be to
> simply use 'js' instead of 'jo' to detect overflows. I'm guessing the only
> reason they don't do that is because some atomic_t's are used to store negative
> values...)
>
> So given that there is an existing solution which AFAICS is efficient and
> achieves the desired protection, why has the proposal turned into a monstrous
> cmpxchg loop that won't be practical to enable by default?
I guess we can try to benchmark the whole thing to see what is the overhead.
Especially now when we have the subsystem parts ready.
And if you have been following the story since beginning, we also have PaX/grsecurity
approach done for linux-next/stable and benchmarks have been previously posted, so
would be easy to compare if needed.
I guess one could in principle think of mixed approach between this one and the one that
PaX/grsecurity had: define refcount_t type and API, but use assembly instructions
behind to speed things up.
> I also think that the "warn when incrementing a 0 refcount" part of the change
> shouldn't be there. It's additional overhead that seems tangential to the main
> goal of the feature which is to protect against refcount overflows, not to
> protect against random increments in some object which has *already* been freed
> and potentially exploited.
Actually having warn for now was useful debugging feature: we got to found out many places which would not work otherwise.
Best Regards,
Elena.
>
> - Eric
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.
