Opened 5 years ago

Closed 3 years ago

#12523 closed enhancement (fixed)

Backport Firefox patch to mark JIT pages as non-writable

Reported by: gk Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-security, tbb-firefox-patch, tbb-hardened, TorBrowserTeam201606R
Cc: intrigeri, tom@…, mcs, brade Actual Points:
Parent ID: #12653 Points:
Reviewer: Sponsor:

Description

We might get away with keeping JIT enabled as long as we mark JIT pages as non-writable. For a patch and the underlying discussion see: https://bugzilla.mozilla.org/show_bug.cgi?id=977805.

Child Tickets

Change History (23)

comment:1 Changed 5 years ago by intrigeri

Cc: intrigeri added

comment:2 Changed 5 years ago by mikeperry

Parent ID: #12653

comment:3 Changed 5 years ago by tom

Cc: tom@… added

comment:4 Changed 5 years ago by erinn

Component: Firefox Patch IssuesTor Browser
Keywords: tbb-firefox-patch added
Owner: changed from mikeperry to tbb-team

comment:5 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201409 added
Owner: changed from tbb-team to arthuredelstein
Status: newassigned

comment:7 Changed 5 years ago by arthuredelstein

I've reverted the patch for now, as I discovered it was causing crashes.

comment:8 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201410 added; TorBrowserTeam201409 removed

comment:9 Changed 5 years ago by mikeperry

Keywords: ff31-esr removed

comment:10 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201410 removed

comment:11 Changed 4 years ago by evilpie

A patch for this just landed in Nightly. Sadly there is no real buildconfig for this, so you might have to just patch ExecutableAllocator::nonWritableJitCode in js/src/jit/ExecutableAllocator.cpp.

comment:12 Changed 4 years ago by mcs

Cc: mcs brade added

comment:13 Changed 4 years ago by gk

Keywords: tbb-hardening added

comment:14 Changed 4 years ago by gk

Keywords: tbb-hardened added; tbb-hardening removed

comment:15 Changed 3 years ago by gk

Keywords: TorBrowserTeam201605 added
Severity: Normal

Mozilla made progress on this and in order to enable that with --non-writable-jitcode we only need to backport https://hg.mozilla.org/mozilla-central/rev/a0dbf1fe665f. Seems to be worth for the next alphas at least. We might want to backport https://bugzilla.mozilla.org/show_bug.cgi?id=1234246 as well.

comment:16 Changed 3 years ago by mcs

Status: assignedneeds_information

I think it makes sense to pick up https://bugzilla.mozilla.org/show_bug.cgi?id=1234246.

We could also backport these two performance fixes that went into FF46:

And there is also the following fix for a somewhat rare crash which will appear in Firefox 48:

gk (or anyone with an opinion): What do you think? My opinion is that we should skip the two performance patches but pick up the crash fix (that patch is fairly simple, but there is some risk because Mozilla has not shipped it yet).

comment:17 in reply to:  16 Changed 3 years ago by gk

Owner: changed from arthuredelstein to mcs
Status: needs_informationassigned

Replying to mcs:

gk (or anyone with an opinion): What do you think? My opinion is that we should skip the two performance patches but pick up the crash fix (that patch is fairly simple, but there is some risk because Mozilla has not shipped it yet).

Sounds good to me and is territory for alpha builds anyway. :)

comment:18 Changed 3 years ago by gk

Keywords: TorBrowserTeam201606 added; TorBrowserTeam201605 removed

comment:19 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201606R added; TorBrowserTeam201606 removed
Status: assignedneeds_review

comment:20 Changed 3 years ago by arthuredelstein

I frankly don't understand the JIT code at all, but the backports here don't have any errors that I can see. If I understand correctly, we need to add a --non-writable-jitcode flag to our mozconfigs for this to take effect.

comment:21 in reply to:  20 ; Changed 3 years ago by mcs

Replying to arthuredelstein:

I frankly don't understand the JIT code at all, but the backports here don't have any errors that I can see. If I understand correctly, we need to add a --non-writable-jitcode flag to our mozconfigs for this to take effect.

At first I agreed with you, but now I do not think we need to add anything to our mozconfigs. Because of the following statement inside js/src/jit/ExecutableAllocator.cpp, this feature is always turned on:

bool ExecutableAllocator::nonWritableJitCode = true;

This is the patch that makes that always so:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug12523-01&id=da628e779cfcbc70e6dde45b09bdc2495d10570a

Or -- I am misunderstanding something.

comment:22 in reply to:  21 Changed 3 years ago by arthuredelstein

Replying to mcs:

Replying to arthuredelstein:

I frankly don't understand the JIT code at all, but the backports here don't have any errors that I can see. If I understand correctly, we need to add a --non-writable-jitcode flag to our mozconfigs for this to take effect.

At first I agreed with you, but now I do not think we need to add anything to our mozconfigs. Because of the following statement inside js/src/jit/ExecutableAllocator.cpp, this feature is always turned on:

bool ExecutableAllocator::nonWritableJitCode = true;

This is the patch that makes that always so:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug12523-01&id=da628e779cfcbc70e6dde45b09bdc2495d10570a

Or -- I am misunderstanding something.

No, I think the misunderstanding was mine. After reading further I think you are correct. Anyhow, these patches looks good as far as I can tell.

comment:23 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Fixed on tor-browser-45.2.0esr-6.5-1 as commit a66ec3f911f8cc247d4c2375e37ac0b9f1377a36, 076919bcd96dcced7020e6003c25425a15ff0cb7 and 2f4d7ae9cfc9d3076a428aa381c6b40bb10166cf.

Note: See TracTickets for help on using tickets.