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

Groups > linux.kernel > #1630417 > unrolled thread

Re: [PATCH] x86/refcount: Implement fast refcount_t handling

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/refcount: Implement fast refcount_t handling "PaX Team" <pageexec@freemail.hu> - 2017-04-25 13:30 +0200
    Re: [PATCH] x86/refcount: Implement fast refcount_t handling Kees Cook <keescook@chromium.org> - 2017-04-25 18:50 +0200
      Re: [PATCH] x86/refcount: Implement fast refcount_t handling "PaX Team" <pageexec@freemail.hu> - 2017-04-26 04:20 +0200
        Re: [PATCH] x86/refcount: Implement fast refcount_t handling Kees Cook <keescook@chromium.org> - 2017-04-26 06:50 +0200

#1630417 — Re: [PATCH] x86/refcount: Implement fast refcount_t handling

On 24 Apr 2017 at 13:33, Kees Cook wrote:

> On Mon, Apr 24, 2017 at 4:00 AM, PaX Team <pageexec@freemail.hu> wrote:
> > On 24 Apr 2017 at 10:32, Peter Zijlstra wrote:
> >
> >> On Fri, Apr 21, 2017 at 03:09:39PM -0700, Kees Cook wrote:
> >> > This patch ports the x86-specific atomic overflow handling from PaX's
> >> > PAX_REFCOUNT to the upstream refcount_t API. This is an updated version
> >> > from PaX that eliminates the saturation race condition by resetting the
> >> > atomic counter back to the INT_MAX saturation value on both overflow and
> >> > underflow. To win a race, a system would have to have INT_MAX threads
> >> > simultaneously overflow before the saturation handler runs.
> >
> > note that the above is wrong (and even contradicting itself and the code).
> 
> True, this changelog could be more accurate (it resets to INT_MAX on
> overflow and INT_MIN on underflow). I think I'm right in saying that a
> system would need INT_MAX threads running a refcount_inc() (and a
> refcount_dec_and_test() at exactly the right moment) before the reset
> handler got scheduled, though, yes?

there's no uniform answer to this as there're several conditions
that can affect the effectiveness of the refcount protection.

e.g., how many independent leaking paths can the attacker exercise
(typically one), are these paths under some kind of locks (would
already prevent unbounded leaks/increments should the overflow
detecting thread be preempted), are negative refcounts allowed and
checked for or only signed overflow, etc.

INT_MAX threads would be needed when the leaking path is locked so
that it can only be exercised once and you'll need to get normal
(balanced) paths preempted just after the increment. if the leaking
path is lockless (can be exercised in parallel without bounds) then
2 threads are enough where the one triggering the signed overflow
would have to be preempted while the other one does INT_MAX increments
and trigger the UAF. this is where the other mechanisms i talked about
in the past become relevant: preemption or interrupts can be disabled
or negative refcount values can be detected and acted upon (your blind
copy-pasting effort passed upon this latter opportunity by not
specializing the 'jo' into 'js' for the refcount case).

[toc] | [next] | [standalone]

#1630782

On Tue, Apr 25, 2017 at 4:26 AM, PaX Team <pageexec@freemail.hu> wrote:
> INT_MAX threads would be needed when the leaking path is locked so
> that it can only be exercised once and you'll need to get normal
> (balanced) paths preempted just after the increment. if the leaking
> path is lockless (can be exercised in parallel without bounds) then
> 2 threads are enough where the one triggering the signed overflow
> would have to be preempted while the other one does INT_MAX increments
> and trigger the UAF. this is where the other mechanisms i talked about
> in the past become relevant: preemption or interrupts can be disabled
> or negative refcount values can be detected and acted upon (your blind
> copy-pasting effort passed upon this latter opportunity by not
> specializing the 'jo' into 'js' for the refcount case).

Well, it's not "blind" -- I'm trying to bring the code as-is to
upstream for discussion/examination with as little functional
differences as possible so it's easier to compare apples to apples.
(Which already resulted in more eyes looking at the code to find a bug
-- thanks Jann!) But yes, jo -> js hugely increases the coverage. I'll
make that change for v2.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1631105

On 25 Apr 2017 at 9:39, Kees Cook wrote:

> On Tue, Apr 25, 2017 at 4:26 AM, PaX Team <pageexec@freemail.hu> wrote:
> > INT_MAX threads would be needed when the leaking path is locked so
> > that it can only be exercised once and you'll need to get normal
> > (balanced) paths preempted just after the increment. if the leaking
> > path is lockless (can be exercised in parallel without bounds) then
> > 2 threads are enough where the one triggering the signed overflow
> > would have to be preempted while the other one does INT_MAX increments
> > and trigger the UAF. this is where the other mechanisms i talked about
> > in the past become relevant: preemption or interrupts can be disabled
> > or negative refcount values can be detected and acted upon (your blind
> > copy-pasting effort passed upon this latter opportunity by not
> > specializing the 'jo' into 'js' for the refcount case).
> 
> Well, it's not "blind" -- I'm trying to bring the code as-is to
> upstream for discussion/examination with as little functional
> differences as possible so it's easier to compare apples to apples.

you copied code from a version which is at least 2 major kernel revisions
behind (so much for those apples), you chose the one version which had a
bug that you didn't spot nor fix properly, you didn't realize the opportunity
that a special refcount type represents, you claimed refcount underflows
aren't exploitable but copied code that would detect signed underflow, you
didn't understand the limits and edge cases i explained above... need i go
on? doesn't leave one with great confidence in your ability to understand
and maintain this code...

[toc] | [prev] | [next] | [standalone]

#1631141

On Tue, Apr 25, 2017 at 7:14 PM, PaX Team <pageexec@freemail.hu> wrote:
> On 25 Apr 2017 at 9:39, Kees Cook wrote:
>
>> On Tue, Apr 25, 2017 at 4:26 AM, PaX Team <pageexec@freemail.hu> wrote:
>> > INT_MAX threads would be needed when the leaking path is locked so
>> > that it can only be exercised once and you'll need to get normal
>> > (balanced) paths preempted just after the increment. if the leaking
>> > path is lockless (can be exercised in parallel without bounds) then
>> > 2 threads are enough where the one triggering the signed overflow
>> > would have to be preempted while the other one does INT_MAX increments
>> > and trigger the UAF. this is where the other mechanisms i talked about
>> > in the past become relevant: preemption or interrupts can be disabled
>> > or negative refcount values can be detected and acted upon (your blind
>> > copy-pasting effort passed upon this latter opportunity by not
>> > specializing the 'jo' into 'js' for the refcount case).
>>
>> Well, it's not "blind" -- I'm trying to bring the code as-is to
>> upstream for discussion/examination with as little functional
>> differences as possible so it's easier to compare apples to apples.
>
> you copied code from a version which is at least 2 major kernel revisions
> behind (so much for those apples)

Hmm, this was from your 4.9 port. Linus hasn't quite released 4.11
yet, so that's actually "at most 2 major kernel revisions behind". :)
Regardless, I'd be happy to refresh the port. Will you share a URL to
your latest rebase against upstream?

> you chose the one version which had a
> bug that you didn't spot nor fix properly, you didn't realize the opportunity
> that a special refcount type represents, you claimed refcount underflows
> aren't exploitable but copied code that would detect signed underflow, you
> didn't understand the limits and edge cases i explained above... need i go

As I said, I was trying to minimize changes to your implementation,
which included the bug and the other issues. The point of this was to
share it with others, and work collaboratively on it. I think this
clearly succeeded with benefits to both upstream and PaX: Jann spotted
the fix for the bug causing weird crashes I saw when doing initial
testing, you pointed out the benefit of using js over jo, I've
reorganized the RMWcc macros for more easily adding trailing
instructions, Peter is thinking about ways around the protection, etc.

> on? doesn't leave one with great confidence in your ability to understand
> and maintain this code...

Well, that's your opinion. I think the patch and its discussion helped
several people, including myself, understand this code. Since many
people will share its maintenance, I think this is the right way to
handle upstreaming these kinds of things. I don't claim to be
omniscient, just persistent. Getting this protection into upstream
means every Linux user will benefit from what you created, which I
think is awesome; this whole class of refcount flaws goes away. Thank
you for writing it, sharing it, and discussing it!

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web