Message-ID: <CAGXu5j+6Ffyx56j_ZmM0Y5j_A_TwssQdRVQ9hX=qTa482r1nvA@mail.gmail.com>
Date:   Fri, 21 Apr 2017 13:22:53 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     "Reshetova, Elena" <elena.reshetova@...el.com>,
        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

On Fri, Apr 21, 2017 at 12:55 PM, Eric Biggers <ebiggers3@...il.com> wrote:
> 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.

We've been trying to handle the conflicting desires of those wanting
very precise refcounting implementation and gaining the security
protections. Ultimately, the best way forward seemed to be to first
land the precise refcounting implementation, and start conversion
until we ran into concerns over performance. Now, since we're here, we
can move forward with getting a fast implementation that provides the
desired security protections without too greatly messing with the
refcount API.

-Kees

-- 
Kees Cook
Pixel Security

Powered by blists - more mailing lists