Date:   Fri, 21 Apr 2017 10:55:29 +0000
From:   "Reshetova, Elena" <elena.reshetova@...el.com>
To:     Eric Biggers <ebiggers3@...il.com>,
        Kees Cook <keescook@...omium.org>
CC:     Christoph Hellwig <hch@...radead.org>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        "james.bottomley@...senpartnership.com" 
        <james.bottomley@...senpartnership.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "fujita.tomonori@....ntt.co.jp" <fujita.tomonori@....ntt.co.jp>,
        "mingo@...hat.com" <mingo@...hat.com>, "clm@...com" <clm@...com>,
        "jbacik@...com" <jbacik@...com>,
        "dsterba@...e.com" <dsterba@...e.com>
Subject: RE: [PATCH 0/5] v2: block subsystem refcounter conversions

> Hi Elena,
> 
> On Thu, Apr 20, 2017 at 04:10:16PM +0000, Reshetova, Elena wrote:
> >
> > > All the objections from DaveM on the amount of cycles spent on the
> > > new refcount_t apply to the block layer fast path operations as well.
> >
> > Ok, could you please indicate the correct way to measure the impact for the
> block layer?
> > We can do the measurements.
> >
> > Best Regards,
> > Elena.
> >
> > >
> > > Please don't send any more conversions until those have been resolved.
> 
> Like I suggested months ago, how about doing an efficient implementation of
> refcount_t which doesn't use the bloated cmpxchg loop?  Then there would be
> no
> need for endless performance arguments.  In fact, in PaX there are already
> example implementations for several architectures.  It's unfortunate that
> they're still being ignored for some reason.

Providing efficient arch. specific implementation for refcount_t is likely the next step
to make performance-sensitive places happy. However, in order to do it, we will need
to measure for each subsystem a) current atomic_t usage - base measurement 
b) pure refcount_t usage impact
c) arch. specific refcount_t impact 

Otherwise I have a chicken and egg problem here: people want better performance
and want to see arch. specific code for this, but in order to convince maintainer
in need of arch. specific code, I need to show that we do have indeed a performance issue. 

So, that's why I was endlessly asking for each subsystem that said that pointed out 
performance reasons to give hints on what should be measured. 
This way we can come back with real numbers (including for arch. specific implement) and
then decide what makes the most sense. 

> 
> At the very least, what is there now could probably be made about twice as fast
> by removing the checks that don't actually help mitigate refcount overflow bugs,
> specifically all the checks in refcount_dec(), and the part of refcount_inc()
> where it doesn't allow incrementing a 0 refcount.  Hint: if a refcount is 0, the
> object has already been freed, so the attacker will have had the opportunity to
> allocate it with contents they control already.

refcount_dec() is used very little through the code actually, it is more like an exception
case since in order to use it one must really be sure that refcounter doesn't drop to zero.
Removing the warn around it wouldn't likely affect much overall and thus it is better to
stay to discourage people of API itself :)

refcount_inc() is of course a different story, it is extensively used. I guess the perf issue
on checking increment from zero might only come from WARNs being printed,
but not really from an additional check here for zero since it is trivial and part of
the needed loop anyway. So, I think only removing the
WARNs might have any visible impact, but even this is probably not really that big. 

So, I think these changes won't really help adoption of interface if arguments against
is performance. If we do have a performance issue, I think arch. specific implementation
is likely the only way to considerably speed it up. 

> 
> Of course, having extra checks behind a debug option is fine.  But they should
> not be part of the base feature; the base feature should just be mitigation of
> reference count *overflows*.  It would be nice to do more, of course; but when
> the extra stuff prevents people from using refcount_t for performance reasons,
> it defeats the point of the feature in the first place.

Sure, but as I said above, I think the smaller tricks and fixes won't be convincing enough,
so their value is questionable. 

> 
> I strongly encourage anyone who has been involved in refcount_t to experiment
> with removing a reference count decrement somewhere in their kernel, then
> trying
> to exploit it to gain code execution.  If you don't know what you're trying to
> protect against, you will not know which defences work and which don't.

Well, we had successful CVEs and even exploits on this in the past. 
@Kees, do you store a list of them in the project?

What do you actually want to verify? The fact that if you manage to create use-after-free
situation, you are able to take advantage of it? 

Best Regards,
Elena.

> 
> - Eric

Powered by blists - more mailing lists