1478036 - kernel32 VirtualAlloc hook jumps to invalid (not mapped) address. Race condition

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[rares.i.nedelcu]

Attachment: windbg.txt

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36

Steps to reproduce:

1. Install latest Firefox available for end users. (61.0.1) ; 32-bit version, 32-bit OS 
2. Install python 2.7
3. Pip install selenium 3.12.0 (latest at the time)
4. Download latest geckodriver (0.21.0)
5.In python console:
from selenium import webdriver
driver = webdriver.Firefox(executable_path="C:\geckodriver.exe") (or path to the geckodriver executable)



Actual results:

It is hard to reproduce.
Initally, kernel32.dll export VirtualAlloc (VirtualAllocStub) starts with:
0:  8b ff                   mov    edi,edi
2:  55                      push   ebp
3:  89 e5                   mov    ebp,esp
5:  5d                      pop    ebp
8b ff is replaced by 2 separate writes, one byte at a time, with a detour jump:
eb f9
Execution occurred after the first byte was written and before write of the second byte:
0:  eb ff                   jmp    0x1
2:  55                      push   ebp
3:  89 e5                   mov    ebp,esp
5:  5d                      pop    ebp

The executed instruction will be:
0:  ff 55 89                call   DWORD PTR [ebp-0x77]

[ebp-0x77] does not contain a valid pointer.

However, when I attached Windbg, it did not catch the moment in-between writes, and displayed the correct final form.
I tested with a proprietary software that can only cause delays in Firefox operations, but does not interact with it any other way. (no control-flow modifications)


Expected results:

The 2 bytes hook should have been written atomically.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This has nothing to do with geckodriver but Firefox' memory allocator, right? So moving it to the appropriate component.

Mike, could you have a look at it? Thanks.

Comment 2

[Mike Hommey [:glandium]]

The only thing we have that does something like this is WindowsDllInterceptor, and AFAIK, we don't use it to intercept VirtualAlloc. David, Aaron, do you confirm?

Comment 3

[(Away)]

Well, we removed the hook of specifically VirtualAlloc in bug 1468207 (currently in Nightly), but in general this bug is valid.

Up through Firefox 60 we used to write `eb f9` as a single 16-bit write: https://dxr.mozilla.org/mozilla-esr60/rev/dd52b41d2b775e5c7261ce52795268b7670635fc/xpcom/build/nsWindowsDllInterceptor.h#323

The DLL interceptor refactoring landed in Firefox 61, and if I understand correctly, we accumulate into a local buffer and memcpy it when we're done. That kinda defeats the nice property of the nop space jump that it can be written atomically. Can we make an exception for this one instruction?

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to David Major [:dmajor] from comment #3)
> That kinda defeats the nice property of the nop space jump that it can be
> written atomically. Can we make an exception for this one instruction?

I suppose we could do something for beta... I guess my main follow-up question is, post-bug 1468207 builds, does that give us anything?

I was intentionally not too concerned about this during the refactoring because we can't do atomic hooks in 64-bit builds. I felt that having differing semantics was not useful (and in fact could induce dangerous assumptions) given that most of our hooks (sans the VirtualAlloc one) are applied in both 32-bit and 64-bit builds.

Bug 1247969 intends to consolidate hook setting to safe places in our code where we can guarantee mutual exclusion during the writing out of our hooks. I'd like to do that in the long term so that these things are non-issues.

David, thoughts?

Comment 5

[(Away)]

The removal of the only(?) 32-bit-specific hook doesn't really change my view of things. If Firefox didn't have an interceptor and we were adding one from scratch in 2018, maybe it would be a convincing argument to say that 32-bit can't have the atomic-write-safety because 64-bit can't. But when we've already had the 32-bit interceptor out in the wild for so many years, to suddenly take away a safety property seems like an unnecessary regression to me.

Also I've kept meaning to make our interceptor support the HotPatch technique from LLVM's codebase, which could make 64-bit accesses safe as well. But I'm so distant from this code now that I'll probably never get around to it.

Anyway, philosophically, I think this bug should be fixed, including for Nightly. But as a practical matter of engineering, if this ends up not being a priority, I'll move on. I don't have any particular passion for pushing on it.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

I'll take a look and see what I can do.

(In reply to David Major [:dmajor] from comment #5)
> Also I've kept meaning to make our interceptor support the HotPatch
> technique from LLVM's codebase, which could make 64-bit accesses safe as
> well. But I'm so distant from this code now that I'll probably never get
> around to it.

If you're not optimistic about doing so yourself, feel free to forward me some docs on this. I'd be willing to look into it.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: bug1478036.patch

This patch adds enough functionality to the interceptor so that inproc NOP-space patchers directly write to their target functions, this preserving atomicity of that write.

Comment 8

[(Away)]

Here's a description of compiler-rt's HotPatch: https://github.com/llvm-mirror/compiler-rt/blob/64c820d819421d8d5dd262b16ed367636e2da23b/lib/interception/interception_win.cc#L62

Comment 9

[David Parks [:handyman]]

Comment on attachment 8995338 [details] [diff] [review]
bug1478036.patch

Review of attachment 8995338 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good for guaranteeing atomicity of this write in 32-bit but I'm not sure I'm clear on what the issue is with 64-bit.  Was the problem that it couldn't be done atomically?  Or is it unnecessary for some reason?  (I see that the reported crash is 32-bit...)

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to David Parks (dparks) [:handyman] from comment #9)
> Patch looks good for guaranteeing atomicity of this write in 32-bit but I'm
> not sure I'm clear on what the issue is with 64-bit.  Was the problem that
> it couldn't be done atomically?  Or is it unnecessary for some reason?  (I
> see that the reported crash is 32-bit...)

The 64-bit interceptor never made any thread-safety guarantees, so the VirtualAlloc hook in question was never applied to 64-bit builds in the first place.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a9ffef86968f14e52b80f953fa71e730002e186
Bug 1478036: Ensure that inproc nop-space patches use atomic writes; r=handyman

Comment 12

[David Parks [:handyman]]

Understood.  I was thinking of this as solving the general "atomic trampoline writing" problem, not just VirtualAlloc.

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/4a9ffef86968

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8995338 [details] [diff] [review]
bug1478036.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1432653
[User impact if declined]: Potential for timing-sensitive crashes on 32-bit Windows builds.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Should be low.
[Why is the change risky/not risky?]:  I know that the interceptor is considered somewhat fragile, but this change isn't too substantial, it just changes how some bytes are written to memory.
[String changes made/needed]: None

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8995338 [details] [diff] [review]
bug1478036.patch

Fix for potential crash, let's uplift for beta 15.

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aaab66845dc5