Closed
Bug 1481518
Opened 6 years ago
Closed 6 years ago
add aarch64 windows support to the sandbox
Categories
(Core :: Security: Process Sandboxing, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: froydnj, Assigned: bobowen)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
23.94 KB,
patch
|
handyman
:
review+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
At a minimum, we need to add some support to this giant #if:
https://searchfox.org/mozilla-central/source/security/sandbox/chromium/build/build_config.h#106-179
I'm sure there are other fun things to start fixing after that.
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Priority: P3 → P1
Assignee | ||
Comment 1•6 years ago
|
||
This patch includes the changes that Microsoft landed for the sandbox along
with other changes to the supporting base files that they depend upon.
Attachment #9030263 -
Flags: review?(davidp99)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #9030265 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•6 years ago
|
||
Look like I need to hone my try fuzzy skills:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1055b7c175edf4db88d1c7d13ed0670b49f33c5c
Reporter | ||
Comment 4•6 years ago
|
||
Comment on attachment 9030265 [details] [diff] [review]
part 2: Enable aarch64 Windows chromium sandbox code
Review of attachment 9030265 [details] [diff] [review]:
-----------------------------------------------------------------
I think you actually want to remove that block in old-configure.in...if you actually need the #define below, we should talk.
::: old-configure.in
@@ +2961,5 @@
> fi
>
> case "$OS_TARGET:$CPU_ARCH" in
> WINNT:aarch64)
> + ARCH_CPU_64_BITS=1
I don't understand why this is necessary, given that the relevant #define is set in your first patch set. Could you explain?
Attachment #9030265 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #4)
...
> ::: old-configure.in
> @@ +2961,5 @@
> > fi
> >
> > case "$OS_TARGET:$CPU_ARCH" in
> > WINNT:aarch64)
> > + ARCH_CPU_64_BITS=1
>
> I don't understand why this is necessary, given that the relevant #define is
> set in your first patch set. Could you explain?
Perhaps I can get rid of that, I'll try.
I added it when I was tracking down that MemoryBarrier problem, because it was showing up in the small amount of code that is used in xul and when I added it, it did change the number of errors, so I thought it was having some effect.
However, since that I realised that I was missing a change to pull in a different atomicops header and the reason I added that define was in the other file anyway.
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Attachment #9030265 -
Attachment is obsolete: true
Assignee | ||
Comment 7•6 years ago
|
||
Comment on attachment 9030273 [details] [diff] [review]
part 2: Enable aarch64 Windows chromium sandbox code
r=froydnj from comment 4.
aarch64 try push with the #define removed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aef23c275a90a2f998572bd50c555cfdb1935615
Attachment #9030273 -
Flags: review+
Comment 8•6 years ago
|
||
Comment on attachment 9030263 [details] [diff] [review]
part 1: Add aarch64 Windows support to the chromium sandbox code
Review of attachment 9030263 [details] [diff] [review]:
-----------------------------------------------------------------
To satisfy my paranoia, I looked up what I could here and everything seems good. Some of it, like the parameters to ReadProcessMemory, are not yet documented aarch64 Windows internals (I guess).
::: security/sandbox/chromium-shim/patches/with_update/add_aarch64_windows_support.patch
@@ +16,5 @@
> + Atomic64 NoBarrier_Load(volatile const Atomic64* ptr);
> + Atomic64 Acquire_Load(volatile const Atomic64* ptr);
> + Atomic64 Release_Load(volatile const Atomic64* ptr);
> + #endif // ARCH_CPU_64_BITS
> +
It looks like there are a few line ending issues with this file but not in the files it applies to. So maybe it applies ok with these lines. But, a warning in case this causes lint issues in the future.
Attachment #9030263 -
Flags: review?(davidp99) → review+
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to David Parks (dparks) [:handyman] from comment #8)
> Comment on attachment 9030263 [details] [diff] [review]
> part 1: Add aarch64 Windows support to the chromium sandbox code
>
> Review of attachment 9030263 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> To satisfy my paranoia, I looked up what I could here and everything seems
> good. Some of it, like the parameters to ReadProcessMemory, are not yet
> documented aarch64 Windows internals (I guess).
That's great, thanks for doing that.
> > + #endif // ARCH_CPU_64_BITS
> > +
>
> It looks like there are a few line ending issues with this file but not in
> the files it applies to. So maybe it applies ok with these lines. But, a
> warning in case this causes lint issues in the future.
Yeah, that's because the control character for an unchanged line is a space, so when you have an empty unchanged line it looks like a trailing whitespace issue.
Comment 10•6 years ago
|
||
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0f7afe6712
part 1: Add aarch64 Windows support to the chromium sandbox code. r=handyman
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f01fb6fbbfd
part 2: Enable aarch64 Windows chromium sandbox code. r=froydnj
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c0f7afe6712
https://hg.mozilla.org/mozilla-central/rev/9f01fb6fbbfd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment 12•6 years ago
|
||
This made the aarch64 builds unusable: nothing loads in any tab. I had to disable the sandbox to get a working browser, although I only tried level 0, not any value between 0 and 5.
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #12)
> This made the aarch64 builds unusable: nothing loads in any tab. I had to
> disable the sandbox to get a working browser, although I only tried level 0,
> not any value between 0 and 5.
glandium, I could reproduce this over the weekend so I landed a patch in bug 1514583 to disable it (it's just hit m-c).
However, I've just tried to reproduce again to start to diagnose the issue and frustratingly it is now working.
Would you re-test your older aarch64 m-c build with the sandbox enabled please.
Flags: needinfo?(mh+mozilla)
Comment 14•6 years ago
|
||
My old aarch64 m-c build was d86d184dc7d6 and with sandbox enabled, it doesn't work. I also tried a7eb50b9dc42, which is presently the last aarch64 build downloadable on m-c, and with sandbox enabled manually, it doesn't work either.
One thing that I noticed, though it hasn't happened recently, is that sometimes Firefox doesn't actually exit when closing windows, and opening what one might think is a new Firefox, actually brings up the one that was still running. Maybe you've hit that? (I now double check every time I close Firefox that there's no process left before I start another one)
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #14)
...
> One thing that I noticed, though it hasn't happened recently, is that
> sometimes Firefox doesn't actually exit when closing windows, and opening
> what one might think is a new Firefox, actually brings up the one that was
> still running. Maybe you've hit that? (I now double check every time I close
> Firefox that there's no process left before I start another one)
Thanks, yeah, I'd seen that too.
I'd copied firefox into |Program Files (Arm)|, which appears to make it work for some reason.
If I try and run it from, for example, Downloads then I get the problem again, permissions I imagine, so it looks like the sandbox rules might not be working after all. :-(
Now I just need to work out what's happening.
Comment 16•6 years ago
|
||
I can confirm it works if I copy the firefox directory into c:\Program Files (Arm)!
Assignee | ||
Comment 17•6 years ago
|
||
The problem here appears to be that it does some part of the DLL loading on a new thread.
The chromium sandbox sets an impersonation token on the main thread of the child process with more rights, so that it can do general start-up things like load DLLs. This is reverted early in the process when we call TargetServicesBase::LowerToken.
I can see the child process spinning up a new thread and calling NtQueryAttributesFile on mozglue.dll, which fails because that thread has the more restricted token.
The brokering won't work either because we haven't called TargetServicesBase::Init and it's possibly too early in the process start-up anyway.
I'll file a new bug and try and work out what we can do about this.
You need to log in
before you can comment on or make changes to this bug.
Description
•