Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1628562

Re: [PATCH 0/5] v2: block subsystem refcounter conversions

Show all headers | View raw

Hi Elena,

On Fri, Apr 21, 2017 at 10:55:29AM +0000, Reshetova, Elena wrote:
> > 
> > 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. 

I should have used refcount_dec_and_test() as the example, as the same applies
to both refcount_dec() and refcount_dec_and_test().  There is negligible
security benefit to have these refcount release operations checked vs. just
calling through to atomic_dec() and atomic_dec_and_test().  It's unfortunate,
but there is no known way to detect ahead of time (i.e. before exploitation) if
there are too many refcount releases, only too many refcount acquires.

The WARN is only executed if there is a bug, so naturally it's only a problem if
the functions are to be inlined, creating bloat.  The actual performance problem
is the overhead associated with using comparisons and cmpxchg's to avoid
changing a refcount that is 0 or UINT_MAX.  The more efficient approach would be
to (a) drop the check for 0, and (b) don't require full operation to be atomic,
but rather do something like "lock incl %[counter]; js <handle_error>".  Yes
it's not "atomic", and people have complained about this, but there is no
technical reason why it needs to be atomic.  Attackers do *not* care whether
your exploit mitigation is theoretically "atomic" or not, they only care whether
it works or not.  And besides, it's not even "atomic_t" anymore, it's
"refcount_t".  

> > 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. 

This makes no sense, as the main point of the feature is supposed to be the
security improvement.  As-is, the extra debugging stuff is actually preventing
the security improvement from being adopted, which is unfortunate.

- Eric

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/5] v2: block subsystem refcounter conversions Elena Reshetova <elena.reshetova@intel.com> - 2017-04-20 13:30 +0200
  [PATCH 2/5] block: convert blk_queue_tag.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-04-20 13:40 +0200
  [PATCH 3/5] block: convert blkcg_gq.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-04-20 13:40 +0200
  Re: [PATCH 0/5] v2: block subsystem refcounter conversions Christoph Hellwig <hch@infradead.org> - 2017-04-20 16:00 +0200
    RE: [PATCH 0/5] v2: block subsystem refcounter conversions "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-04-20 18:20 +0200
      Re: [PATCH 0/5] v2: block subsystem refcounter conversions Eric Biggers <ebiggers3@gmail.com> - 2017-04-20 20:40 +0200
        RE: [PATCH 0/5] v2: block subsystem refcounter conversions "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-04-21 13:00 +0200
          Re: [PATCH 0/5] v2: block subsystem refcounter conversions Jens Axboe <axboe@kernel.dk> - 2017-04-21 16:10 +0200
            Re: [PATCH 0/5] v2: block subsystem refcounter conversions Jens Axboe <axboe@kernel.dk> - 2017-04-21 19:10 +0200
            Re: [PATCH 0/5] v2: block subsystem refcounter conversions Peter Zijlstra <peterz@infradead.org> - 2017-04-21 19:10 +0200
          Re: [PATCH 0/5] v2: block subsystem refcounter conversions Kees Cook <keescook@chromium.org> - 2017-04-21 21:30 +0200
          Re: [PATCH 0/5] v2: block subsystem refcounter conversions Eric Biggers <ebiggers3@gmail.com> - 2017-04-21 22:00 +0200
            Re: [PATCH 0/5] v2: block subsystem refcounter conversions Kees Cook <keescook@chromium.org> - 2017-04-21 22:30 +0200
              Re: [PATCH 0/5] v2: block subsystem refcounter conversions James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-04-21 23:30 +0200
                Re: [PATCH 0/5] v2: block subsystem refcounter conversions Kees Cook <keescook@chromium.org> - 2017-04-21 23:40 +0200
                Re: [PATCH 0/5] v2: block subsystem refcounter conversions James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-04-22 00:10 +0200
        Re: [PATCH 0/5] v2: block subsystem refcounter conversions Christoph Hellwig <hch@infradead.org> - 2017-04-21 13:00 +0200

csiph-web