<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Assignee

Comment 2

•

9 months ago

Attached file min_poc.js — Details

Assignee

Comment 3

•

9 months ago

I've attached a minimal reproducer.

$ ./obj/dist/bin/js --ion-eager min_poc.js
[1769997] Assertion failure: is_intn(imm, kJumpOffsetBits), at /home/mantle/workspace/mozilla-firefox-dev/js/src/jit/riscv64/extension/base-riscv-i.h:113
[...]

This seems to be because we place the veneer code too far away from the last branch, thus overflowing the jal immediate:

In file: /home/mantle/workspace/mozilla-firefox-dev/js/src/jit/riscv64/extension/base-riscv-i.h:113
   108   static inline Instr SetJalOffset(int32_t pos, int32_t target_pos,
   109                                    Instr instr) {
   110     MOZ_ASSERT(IsJal(instr));
   111     int32_t imm = target_pos - pos;
   112     MOZ_ASSERT((imm & 1) == 0);
 ► 113     MOZ_ASSERT(is_intn(imm, kJumpOffsetBits));

pwndbg> print /x imm
$1 = 0x100000
pwndbg> print /x target_pos
$2 = 0x100b34
pwndbg> print /x pos
$3 = 0xb34

0x100000 barely overflows the jump target range (^1). I think we may need to shrink the constant pools a bit, so that it leaves some space for the veneer.

On security: When this assert is disabled in non-debug builds, this jump is assembled as jal rd, -1048576, jumping backwards instead of forwards.

Flags: needinfo?(webmaster)

Assignee

Comment 4

•

9 months ago

Adding --riscv-debug shows that the veneer itself (0x100950) is behind branch deadline (0x10094f). This surely can't be right :)

        PatchShortRangeBranchToVeneer
        0x52500004e2d4(94c): 0040006f       j         4 -> 0x7f8957e20e24
         instert veneer 100948, branch:94c deadline: 10094b
         target_at: 0x52500004e2d4(94c)
        0040006f       j         4 -> 0x7ffdcad5f1a4
         target_at: 4 2384
        fix to veneer:7fdff06f       j         1048572 -> 0x7ffdcae5f43c
        PatchShortRangeBranchToVeneer
        0x52500004e2d8(950): 0000006f       j         0 -> 0x7f8957e21020
         instert veneer 100950, branch:950 deadline: 10094f
         target_at: 0x52500004e2d8(950)
        0000006f       j         0 -> 0x7ffdcad5f1a0

Frederik Braun [:freddy] (OOO - back August 10th)

Assignee

Comment 5

•

9 months ago

Whenever a veneer is inserted, it extends the "used" memory area forward (^1). When this happens to cross a second short jump's deadline, we'll overflow its offset imm to the veneer, cancelling out the veneer's purpose.

My first (WIP) approach is to reserve veneer space for such indirectly expiring jumps in (^2). This would be an iteration on branchDeadlines_, accumulating the reserved space until no new expiring jumps are found. It would be expensive thanks to the structure of BranchDeadlineSet.

Another approach would be eliminating AssemblerBufferWithConstantPools from riscv64 code completely. I'm not sure whether ARM[64] has this bug as well, but neither of mips64 and loong64 is using it.

The original author of Bug 1210554 (:jolesen) doesn't seem active now, so I'd be happy if I could use some help from someone with better experiences on AssemblerBufferWithConstantPools.

Flags: needinfo?(sdetar)

Updated

•

9 months ago

Flags: sec-bounty?

Steven DeTar [:sdetar]

Comment 6

•

9 months ago

Rong, I am looking for the right person to help you. I will get back to you quickly.

Daniel Veditz [:dveditz]

Updated

•

9 months ago

Keywords: csectype-jit, sec-high

Hardware: Unspecified → RISCV64

BugBot [:suhaib / :marco]

Comment 7

•

9 months ago

The severity field for this bug is set to S4. However, the bug is flagged with the sec-high keyword.
:sdetar, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Flags: needinfo?(sdetar)

Steven DeTar [:sdetar]

Comment 8

•

9 months ago

Rong, Nicolas (needinfo'd here) from the SpiderMonkey team should be able help you with this

Flags: needinfo?(sdetar)

Flags: needinfo?(nicolas.b.pierron)

Comment 9

•

9 months ago

So far I cannot reproduce this issue on ARM64 and I failed to test the ARM simulator due to rustc build errors.

The problem seems to come from the way the secondaryVeneers is computed in hasSpaceForInsts. This code got added in the past to handle cases where multiple instructions with different reach-sizes could have deadlines overlapping each others. Leading to the current computation of the secondaryVeneers.

However, the code does not consider the case where the guardSize_ would be larger than the initial branch. Leading to this bug. The minimal spacing of branches should probably be considered in the computation of the secondaryVeneers to fix this issue.

This problem should not affect Arm, nor Arm64, which both have a guardSize_ of 1.

Flags: needinfo?(nicolas.b.pierron)

Assignee

Comment 10

•

9 months ago

Thanks for the explanation.

Another observation is that while deadline (^1), poolEnd (^2) are both counted in bytes, secondaryVeneers seems to be counted in Inst. I think we are also missing a * InstSize at (^3), right? Perhaps we should also mark this bug as regressed by Bug 1210554?

Flags: needinfo?(nicolas.b.pierron)

Assignee

Comment 11

•

9 months ago

Attached file (secure) — Details

Phabricator Automation

Updated

•

9 months ago

Assignee: nobody → webmaster

Status: UNCONFIRMED → ASSIGNED

Ever confirmed: true

Assignee

Comment 12

•

9 months ago

Attached file (secure) — Details

Assignee

Comment 13

•

9 months ago

Attached file (secure) — Details

MIPS is no longer using AssemblerBufferWithConstantPools while RISCV64
is currently using it, according to
https://searchfox.org/firefox-main/search?q=AssemblerBufferWithConstantPools&path=&case=true&regexp=false.

Lingming Zhang

Reporter

Comment 14

•

9 months ago

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #7)

The severity field for this bug is set to S4. However, the bug is flagged with the sec-high keyword.
:sdetar, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Truly appreciate the time and effort developers have paid for fixing this bug! I noticed that this bug was initially set to P5 and S4 which may seem inappropriate for now. I wonder if it is possible to change these two fields to help me (and late-comers) to better understand the significance of this bug, thanks!

Assignee

Comment 15

•

9 months ago

Can we insert more than one veneers for a single branch? I'm trying to tackle this bug by reducing riscv64's veneer to a single j offset (aka jal zero, offset) with a 21-bit immediate (1MiB range), but one veneer isn't enough for some jit-tests (thus real world cases).

ARM[64]'s veneer is a single b imm (^1) with a 26-bit immediate (^2) (32MiB range), so it does have potential to overflow. I wonder how we deal with this currently in ARM[64]?

Andrew McCreight [:mccr8]

Comment 16

•

9 months ago

•

Edited

(In reply to Rong Bao [:csmantle] from comment #10)

Thanks for the explanation.

Another observation is that while deadline (^1), poolEnd (^2) are both counted in bytes, secondaryVeneers seems to be counted in Inst. I think we are also missing a * InstSize at (^3), right? Perhaps we should also mark this bug as regressed by Bug 1210554?

This is indeed correct! Good Catch!

And for what is worth, this could potentially be leveraged on ARM64 as well.

This issue is being addressed by this patch attached on this bug, and we should consider back-porting it to all branches on a release train of Firefox.

Multiple branch sizes could be registered, such that one of the veneer (trampoline between out-of-reached regions) can be corrupted to encode an address which is incorrect, leading to a jump at an unexpected location on ARM64.

[edit: remove the fact that this can be leveraged on ARM, given that ARM no longer seems to be using veneer]

Severity: S4 → S2

Flags: needinfo?(nicolas.b.pierron)

Keywords: sec-high → sec-critical

Priority: P5 → P1

Hardware: RISCV64 → All

Comment 17

•

9 months ago

This doesn't need to be sec-critical, unless there is a known public exploit.

Keywords: sec-critical → sec-high

Comment 18

•

9 months ago

(In reply to Rong Bao [:csmantle] from comment #15)

Can we insert more than one veneers for a single branch? I'm trying to tackle this bug by reducing riscv64's veneer to a single j offset (aka jal zero, offset) with a 21-bit immediate (1MiB range), but one veneer isn't enough for some jit-tests (thus real world cases).

ARM[64]'s veneer is a single b imm (^1) with a 26-bit immediate (^2) (32MiB range), so it does have potential to overflow. I wonder how we deal with this currently in ARM[64]?

ARM was supposed to chain veneer, but I notice that the current ARM implementation does not use veneer anymore.

ARM64 indeed uses a 26 bit immediate, which should be sufficient to address all addressed in the allocated JIT code region

Comment 19

•

9 months ago

(In reply to Lingming Zhang from comment #14)

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #7)

The severity field for this bug is set to S4. However, the bug is flagged with the sec-high keyword.
:sdetar, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Truly appreciate the time and effort developers have paid for fixing this bug! I noticed that this bug was initially set to P5 and S4 which may seem inappropriate for now. I wonder if it is possible to change these two fields to help me (and late-comers) to better understand the significance of this bug, thanks!

This was set to P5 — no expected landing deadline —, and S4 — no impact in Tier-1 platform.

This has been changed to P1 — expect this change the the upcoming release —, and S2 — impact Tier-1 platform (ARM64) and no work-around exists for this issue.

The reason for sec-critical / sec-high was that this could be used as a reliable way to get random code execution on ARM64.

Comment 20

•

9 months ago

Rong, if you need any guidance for the procedure to follow for landing security issues, feel free to needinfo anybody from the team.
Otherwise, if you don't feel like going through this procedure, you can also delegate ;)

Flags: needinfo?(webmaster)

Assignee

Comment 21

•

9 months ago

(In reply to Nicolas B. Pierron [:nbp] from comment #18)

ARM was supposed to chain veneer, but I notice that the current ARM implementation does not use veneer anymore.

Thanks. I'll take a look at ARM's veneer chaining and port that to RV64, so that we can use single-jal branches, as well as avoiding much performance penalty of sorting as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1210554#c20.

(In reply to comment #20)

Rong, if you need any guidance for the procedure to follow for landing security issues, feel free to needinfo anybody from the team.
Otherwise, if you don't feel like going through this procedure, you can also delegate ;)

Sure, I would call out for help if needed, thanks. I'm fine with the procedures. Also I'm on GMT+8, so I will get back and update as soon as possible.

Flags: needinfo?(webmaster)

Assignee

Comment 22

•

9 months ago

Attached file (secure) — Details

Assignee

Updated

•

9 months ago

Attachment #9523086 - Attachment mime type: text/plain → text/javascript

Assignee

Comment 23

•

9 months ago

Current patch state:

arm64-simulator: jit-test --ion, jstests, and jsapi-tests are all green with the whole stack applied
riscv64-simulator: jstests and jsapi-tests are both green. jit-test --ion has 6 failures, all of which asserted with next != LabelBase::INVALID_OFFSET, at /home/mantle/workspace/mozilla-firefox-dev/js/src/jit/riscv64/Assembler-riscv64.cpp:1070. It seems that riscv64's assembler can't insert multiple veneers when the first still can't reach the target.

Assignee

Comment 24

•

9 months ago

Attached file (secure) — Details

Assignee

Comment 25

•

9 months ago

•

Edited

I find the riscv64 part more complex than I thought.

Finding an exact solution for d[i] - d[0] >= i * branchSize as in Bug 1210554, comment 20 (current differentials) is very expensive (x2.5 total jit-test time on riscv64-simulator), and also causing inexplicable Assertion failure: inhibitPools_ == 0, at /home/mantle/workspace/mozilla-firefox-dev/js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:1020.
Implementing single-instruction veneer requires a working veneer chaining, which Bug 1800431 didn't provide, also with no working examples (see comment 18). This approach's current status is described in comment 23.

However, the arm64 part is trivial to fix. I would propose splitting this bug into two, so that the more critical arm64 part can be addressed swiftly, as well as allocating more time to develop the patch for the less urgent riscv64 part.

Assignee

Updated

•

9 months ago

Flags: needinfo?(nicolas.b.pierron)

Comment 26

•

9 months ago

(In reply to Rong Bao [:csmantle] from comment #25)

I find the riscv64 part more complex than I thought.

I was about to comment about it.

The first order approximation is to assume that we need space for all branches and that we should start when the veneer size exceeds the maximal jump size.

      size_t firstOrderMargin = guardSize_ * branchDeadlines_.size() * InstSize;

A second order approximation, is we are about to do, to consider the fact that branches from one category are also taking code space to implement to source-jump, and that branches with the same range cannot alias each others.

      size_t secondOrderMargin = firstOrderMargin - guardSize_ * branchDeadlines_.maxRangeSize() * InstSize;

Here maxRangeSize selects one category of branch range as the one which has the most branches. Then the guardSize_ factor sounds incorrect, as we want to remove bytes for the fact that the source of branches, which cannot alias each others, are taking some space.

Thus guardSize_ * branchDeadlines_.maxRangeSize() should probably be replaced by a new function which returns the largest size of source branches. The size of the source branches is not guardSize_. This should be the minimal spacing between 2 branches. I guess this is something which can be asserted by comparing against the spacing between 2 consecutive deadlines when a new branch is inserted.

A third order approximation would indeed be to consider every branches to find the shortest deadline. If we wanted to go on that road, maybe the simplest approach would probably be to pre-build the veneer allocations, by having a vector of veneer in which we perform an insert-sort based on the deadline. Thus the margin becomes the smallest gap between the deadline and the veneer index deadline[veneerIndex] - veneerIndex * guardSize_ in the vector of all branches.

I think the second order margin is still a good approximation.

Flags: needinfo?(nicolas.b.pierron)

Assignee

Comment 27

•

8 months ago

Attached file (secure) — Details

Assignee

Comment 28

•

8 months ago

•

Edited

(In reply to Nicolas B. Pierron [:nbp] {back on 17/11} from comment #26)

I think the second order margin is still a good approximation.

Hi Nicolas, I've managed to get a third-order solution working (as a PoC for other changes). I'm now working on the cost-efficient second-order approximation as you've suggested.

Assignee

Comment 30

•

8 months ago

•

Edited

With experimentation, I observe that the second-order approximation is still too pessimistic and coarse. When the number of deadlines is large, the pool will try to flush ad infinitum even when it's already empty, especially with bigint/bigint-*.js tests:

[Pools] Attempting to finish pool 0 with 0 entries.
[Pools] branchDeadlines_.size()=732, branchDeadlines_.maxRangeSize()=732
[Pools] Aborting because the pool is empty
[Pools] Inserting instruction(1047064) caused a spill
[Pools] Attempting to finish pool 0 with 0 entries.
[Pools] branchDeadlines_.size()=732, branchDeadlines_.maxRangeSize()=732
[Pools] Aborting because the pool is empty
[Pools] Inserting instruction(1047064) caused a spill
[Pools] Attempting to finish pool 0 with 0 entries.
[Pools] branchDeadlines_.size()=732, branchDeadlines_.maxRangeSize()=732
[Pools] Aborting because the pool is empty
[Pools] Inserting instruction(1047064) caused a spill
[Pools] Attempting to finish pool 0 with 0 entries.
...

So we might have to bite the bullet and implement a more efficient third-order solution. Existing access pattern heuristics in addDeadline (^1) and removeDeadline (^2) may be reused here.

Update: Make spew message clearer.

Comment 31

•

8 months ago

(In reply to Rong Bao [:csmantle] from comment #30)

With experimentation, I observe that the second-order approximation is still too pessimistic and coarse. When the number of deadlines is large, the pool will try to flush ad infinitum even when it's already empty, especially with bigint/bigint-*.js tests:

I have a pragmatic questions …

How much code do we need for each approximations?
How likely is it to be a problem in user web browsing?

I would be hesitant to accept a lot of code, given that this is not exercised on Tier-1 platforms. On the other hand we could disabled the compilation of functions, if this is a problem restricted to a small percent of Ion-compiled functions. JIT compilation is an optimization, and the code should still run safely without any JIT optimizations.

Also, as this is a security issue, we should probably focus on the solution which disable the JIT when simpler implementation cannot generate safe code, instead of focusing on a third-order approximation. (sorry for the late reply)

Assignee

Comment 32

•

8 months ago

•

Edited

(In reply to Nicolas B. Pierron [:nbp] from comment #31)

I have a pragmatic questions …

How much code do we need for each approximations?

First- and second-order approximation are almost trivial to implement with very minimal code changes.

My current third-order approximation needs to refactor BranchDeadlineSet into a unified deadline vector (as opposed to the current bucketing by range index). The core changes (Part 2) would be +132, -130, and I agree that this is not a small change.

The current patchset can pass all {arm, arm64, riscv64}-simulator × {jit-tests [--ion], jstests, jsapi-tests}, though the riscv64 jstests are slower than before. I'm currently conducting tests on the exact numbers of performance regression.

How likely is it to be a problem in user web browsing?

Frankly, I don't have a very comprehensive overview of real-world payloads of Fx on riscv64. But personally, I didn't have much problem using Fx 145 in my daily surfing, including watching videos on Bilibili and running https://browserben.ch.

I would be hesitant to accept a lot of code, given that this is not exercised on Tier-1 platforms. On the other hand we could disabled the compilation of functions, if this is a problem restricted to a small percent of Ion-compiled functions. JIT compilation is an optimization, and the code should still run safely without any JIT optimizations.

I understand. Would you mind elaborating more on how to abort compilation in these circumstances? I know that at runtime it's possible to bail out to lower tiers, but I'm not sure how to do this when compiling.

Also, as this is a security issue, we should probably focus on the solution which disable the JIT when simpler implementation cannot generate safe code, instead of focusing on a third-order approximation. (sorry for the late reply)

It's okay, and thanks so much for your comments.

Comment 33

•

8 months ago

•

Edited

(In reply to Rong Bao [:csmantle] from comment #32)

(In reply to Nicolas B. Pierron [:nbp] from comment #31)

I would be hesitant to accept a lot of code, given that this is not exercised on Tier-1 platforms. On the other hand we could disabled the compilation of functions, if this is a problem restricted to a small percent of Ion-compiled functions. JIT compilation is an optimization, and the code should still run safely without any JIT optimizations.

I understand. Would you mind elaborating more on how to abort compilation in these circumstances? I know that at runtime it's possible to bail out to lower tiers, but I'm not sure how to do this when compiling.

This is quite simple, just set the OOM flag of the Assembler using masm.setOOM(), and top-level loops should already check this flag to prematurely end the compilation, such as the returned value of CodeGenerator::generateBlock, this error then bubbles up to disable further compilation of this function.

Assignee

Comment 34

•

8 months ago

•

Edited

(In reply to Nicolas B. Pierron [:nbp] from comment #33)

This is quite simple, just set the OOM flag of the Assembler using masm.setOOM(), and top-level loops should already check this flag to prematurely end the compilation, such as the returned value of CodeGenerator::generateBlock, this error then bubbles up to disable further compilation of this function.

Thanks for the direction, but it seems that masm.setOOM throws an exception in the end, instead of continuing in the interpreter. I'll attach a WIP patch file later, as I'm not quite sure whether my current implementation is on the right track.

[...]
[Pools] No-Pool instruction(1047008) caused a spill.
[Pools] Attempting to finish pool 0 with 0 entries.
[Pools] Aborting because the pool is empty
[Pools] No free space even after flush, must be in a flush loop
[Codegen] OOM!
[Pools] No-Pool instruction(1047008) caused a spill.
[Pools] Attempting to finish pool 0 with 0 entries.
[Pools] Aborting because the pool is empty
[Pools] No free space even after flush, must be in a flush loop
[Codegen] OOM!
[Codegen] # END   creators: BaselineCompiler::compile/BaselineCodeGen<Handler>::emitOutOfLinePostBarrierSlot
[Codegen] # END   creators: BaselineCompiler::compile
[Codegen] # BEGIN creators: exception_tail
[Codegen] # END   creators: exception_tail
uncaught exception: out of memory

Aside from that, here are some preliminary (and coarse) perf results for the current 3rd-order solution:

riscv64-simulator (disable-debug, enable-optimize)

Before:

$ hyperfine --runs 3 'MOZCONFIG=./mozconfig.nodebug ./mach jit-test --ion -t 3600'
Benchmark 1: MOZCONFIG=./mozconfig.nodebug ./mach jit-test --ion -t 3600
  Time (mean ± σ):     81.118 s ±  0.126 s    [User: 1912.632 s, System: 98.675 s]
  Range (min … max):   81.004 s … 81.253 s    3 runs

After:

$ hyperfine --runs 3 'MOZCONFIG=./mozconfig.nodebug ./mach jit-test --ion -t 3600'
Benchmark 1: MOZCONFIG=./mozconfig.nodebug ./mach jit-test --ion -t 3600
  Time (mean ± σ):     79.648 s ±  0.096 s    [User: 1908.845 s, System: 99.048 s]
  Range (min … max):   79.580 s … 79.758 s    3 runs

arm64-simulator (disable-debug, enable-optimize)

Before:

$ hyperfine --runs 3 'MOZCONFIG=./mozconfig.arm64-simulator ./mach jit-test --ion -t 3600'
Benchmark 1: MOZCONFIG=./mozconfig.arm64-simulator ./mach jit-test --ion -t 3600
  Time (mean ± σ):     68.362 s ±  0.189 s    [User: 1779.438 s, System: 98.741 s]
  Range (min … max):   68.144 s … 68.487 s    3 runs

After:

$ hyperfine --runs 3 'MOZCONFIG=./mozconfig.arm64-simulator ./mach jit-test --ion -t 3600'
Benchmark 1: MOZCONFIG=./mozconfig.arm64-simulator ./mach jit-test --ion -t 3600
  Time (mean ± σ):     69.255 s ±  0.110 s    [User: 1794.218 s, System: 98.657 s]
  Range (min … max):   69.155 s … 69.372 s    3 runs

riscv64 native (enable-debug, enable-optimize)

Before:

$ ./mach jit-test --ion -t 3600
[21773|    0|    0| 1348] 100% ======================================>|4403.4s
PASSED ALL

After:

$ ./mach jit-test --ion -t 3600
Creating the 'common' site at /home/csmantle/.mozbuild/srcdirs/mozilla-firefox-dev-2ddf79ca1d57/_virtualenvs/common
[21773|    0|    0| 1348] 100% ======================================>|4507.4s
PASSED ALL

Assignee

Comment 35

•

8 months ago

Attached patch WIP-0002.patch (obsolete) — Details — Splinter Review

WIP 2nd-order patch to replace Part 2 in the stack

Comment 36

•

8 months ago

(In reply to Rong Bao [:csmantle] from comment #34)

(In reply to Nicolas B. Pierron [:nbp] from comment #33)

This is quite simple, just set the OOM flag of the Assembler using masm.setOOM(), and top-level loops should already check this flag to prematurely end the compilation, such as the returned value of CodeGenerator::generateBlock, this error then bubbles up to disable further compilation of this function.

Thanks for the direction, but it seems that masm.setOOM throws an exception in the end, instead of continuing in the interpreter. I'll attach a WIP patch file later, as I'm not quite sure whether my current implementation is on the right track.
[...]
[Pools] No-Pool instruction(1047008) caused a spill.
[Pools] Attempting to finish pool 0 with 0 entries.
[Pools] Aborting because the pool is empty
[Pools] No free space even after flush, must be in a flush loop
[Codegen] OOM!
[Pools] No-Pool instruction(1047008) caused a spill.
[Pools] Attempting to finish pool 0 with 0 entries.
[Pools] Aborting because the pool is empty
[Pools] No free space even after flush, must be in a flush loop
[Codegen] OOM!
[Codegen] # END   creators: BaselineCompiler::compile/BaselineCodeGen<Handler>::emitOutOfLinePostBarrierSlot
[Codegen] # END   creators: BaselineCompiler::compile
[Codegen] # BEGIN creators: exception_tail
[Codegen] # END   creators: exception_tail
uncaught exception: out of memory

This is the BaselineCompiler and not IonMonkey. Interesting.
This explains why we did not consider the oom as an Abort::Disable case and why an exception is raised.

Maybe we can use longer spanning branches for Baseline code?

Aside from that, here are some preliminary (and coarse) perf results for the current 3rd-order solution:

Thanks for checking, I would not expect anything from benchmarking the test suite harness.
I also have a hard time believing the reported variance on such a lengthy tasks. This sounds surprisingly low from my point of view.

Also, this probably belong in a follow-up bug, and keep this bug focused on the security issue at hand.

(In reply to Rong Bao [:csmantle] from comment #35)

Created attachment 9527422 [details] [diff] [review]
WIP-0002.patch

The out-param feels a bit messy. The AssemblerBuffer also has a setOOM function, which should be reflected by the masm.oom() function.

Assignee

Comment 37

•

8 months ago

(In reply to Nicolas B. Pierron [:nbp] from comment #36)

Maybe we can use longer spanning branches for Baseline code?

I think we can just insert a nop after every short branch to make them as long as veneers. That would probably be a quick and dirty workaround.

Thanks for checking, I would not expect anything from benchmarking the test suite harness.
I also have a hard time believing the reported variance on such a lengthy tasks. This sounds surprisingly low from my point of view.

Also, this probably belong in a follow-up bug, and keep this bug focused on the security issue at hand.

Sure. I'll put those implementations into some follow-up bug after this one is finished.

Assignee

Comment 38

•

8 months ago

•

Edited

I think the patchset is RFR.

The approach taken here is to pad all short branches:

b[cc] to b[cc]; nop
jal to jal; nop

... so that they are as long as veneers, and decompression is beyond concern.

Relevant changes to shared code are done to:

Get and register the correct return address for the new jal; nop sequence.
Account for potential new deadlines to be registered in no-pool areas, so that they don't cause a spill in the inhibited area.
Collect some spread-out bitwise operations to helper functions.
Docs update.

Nicolas, would you mind to kindly give this a review?

Flags: needinfo?(nicolas.b.pierron)

Comment 39

•

8 months ago

(In reply to Rong Bao [:csmantle] from comment #38)

Nicolas, would you mind to kindly give this a review?

These patches sounds good to me.

You can proceed with sec-approval, as mentioned previously.

Flags: needinfo?(nicolas.b.pierron)

Assignee

Comment 40

•

8 months ago

(In reply to Nicolas B. Pierron [:nbp] from comment #39)

These patches sounds good to me.

You can proceed with sec-approval, as mentioned previously.

Thanks so much. I'm adding regression flags to get a list of affected versions for use in sec-approval form.

Keywords: regression

Regressed by: 1210554

BugBot [:suhaib / :marco]

Assignee

Updated

•

8 months ago

Attachment #9527422 - Attachment is obsolete: true

Comment 41

•

8 months ago

Set release status flags based on info from the regressing bug 1210554

status-firefox145: --- → affected

status-firefox146: --- → affected

status-firefox147: --- → affected

status-firefox-esr140: --- → affected

Assignee

Comment 42

•

8 months ago

Comment on attachment 9523928 [details]
(secure)

Security Approval Request

How easily could an exploit be constructed based on the patch?: Not easily. The riscv64 part's intention is not easily deduced. This patchset is already obfuscating its intention by framing arm64 fixes as "Drive-by", and mixing the actual fix with other defensive and SMDOC cleanups.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All supported branches
If not all supported branches, which bug introduced the flaw?: None
Do you have backports for the affected branches?: No
If not, how different, hard to create, and risky will they be?: Low difficulty and low risks.

the code change for arm64 is minimal and easy to port.
riscv64 is a Tier-3 architecture, and the patchset changes low-churn code areas.

How likely is this patch to cause regressions; how much testing does it need?: Low regression risks.

For arm64, the code change is merely a fix, and all local jit-tests --ion, jstests and jsapi-tests are green.
For riscv64, it's a Tier-3 arch, and all said local tests are green.
For changes in shared code, the behavior of other archs is not changed, thus unlikely to cause regressions. Besides, local tests are all green on x64 native.

Is the patch ready to land after security approval is given?: Yes
Is Android affected?: Yes

Attachment #9523928 - Flags: sec-approval?

Assignee

Comment 43

•

8 months ago

Should I request sec-approval on every diff?

Assignee

Comment 44

•

8 months ago

Comment on attachment 9523929 [details]
(secure)

Security Approval Request

How easily could an exploit be constructed based on the patch?: Not likely. This is only a SMDOC cleanup that removes MIPS from constant pooling users and adds RISCV64 to it, without mentioning anything about implementation.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All branches
If not all supported branches, which bug introduced the flaw?: None
Do you have backports for the affected branches?: No
If not, how different, hard to create, and risky will they be?: This is trivial to backport thanks to being a docs change.
How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It's a docs change.
Is the patch ready to land after security approval is given?: Yes
Is Android affected?: Unknown

Attachment #9523929 - Flags: sec-approval?

Assignee

Comment 45

•

8 months ago

Comment on attachment 9524366 [details]
(secure)

Security Approval Request

How easily could an exploit be constructed based on the patch?: Unlikely. This patch contains mostly defensive changes added while debugging.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All branches
If not all supported branches, which bug introduced the flaw?: None
Do you have backports for the affected branches?: No
If not, how different, hard to create, and risky will they be?: Not likely to be hard. This changes low-churn code in riscv64 (Tier-3) support only.
How likely is this patch to cause regressions; how much testing does it need?: Unlikely; see Bug 1996840 comment 42.
Is the patch ready to land after security approval is given?: Yes
Is Android affected?: No

Attachment #9524366 - Flags: sec-approval?

Assignee

Comment 46

•

8 months ago

Comment on attachment 9525288 [details]
(secure)

Security Approval Request

How easily could an exploit be constructed based on the patch?: Unlikely. This patch only changes incomprehensible debug printouts in riscv64 (Tier-3) backend.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All branches
If not all supported branches, which bug introduced the flaw?: None
Do you have backports for the affected branches?: No
If not, how different, hard to create, and risky will they be?: Not hard nor risky. It's only a debugging printout change.
How likely is this patch to cause regressions; how much testing does it need?: Unlikely; see Bug 1996840 comment 42.
Is the patch ready to land after security approval is given?: Yes
Is Android affected?: No

Attachment #9525288 - Flags: sec-approval?

Assignee

Comment 47

•

8 months ago

Comment on attachment 9525570 [details]
(secure)

Security Approval Request

How easily could an exploit be constructed based on the patch?: Not easily. This patch adds adjustments to no-pool areas according to potential deadlines, but the direct relationship to the bug is not easily deduced.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All branches.
If not all supported branches, which bug introduced the flaw?: None
Do you have backports for the affected branches?: No
If not, how different, hard to create, and risky will they be?: Not hard. Changes are in low-churn areas so this patch is likely to be reusable.
How likely is this patch to cause regressions; how much testing does it need?: Unlikely; see Bug 1996840 comment 42.
Is the patch ready to land after security approval is given?: Yes
Is Android affected?: No

Attachment #9525570 - Flags: sec-approval?

Ryan VanderMeulen [:RyanVM]

Assignee

Comment 48

•

8 months ago

Comment on attachment 9523927 [details]
(secure)

Security Approval Request

How easily could an exploit be constructed based on the patch?: From this sole patch it's not easy to deduce an exploit, since it just pads each short branch as a workaround. But with extensive knowledge about constant pooling, one could potentially infer the relationship with other patches and develop an exploit.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All branches
If not all supported branches, which bug introduced the flaw?: None
Do you have backports for the affected branches?: No
If not, how different, hard to create, and risky will they be?: Changes are in low-churn code areas, so this patch could likely be reused.
How likely is this patch to cause regressions; how much testing does it need?: Unlikely; see Bug 1996840 comment 42.
Is the patch ready to land after security approval is given?: Yes
Is Android affected?: No

Attachment #9523927 - Flags: sec-approval?

Updated

•

8 months ago

status-firefox145: affected → wontfix

status-firefox-esr115: --- → affected

tracking-firefox146: --- → +

tracking-firefox147: --- → +

tracking-firefox-esr115: --- → 146+

tracking-firefox-esr140: --- → 146+

Comment 49

•

8 months ago

Comment on attachment 9523928 [details]
(secure)

Approved to confirm with relman they're okay with uplifting, and if they are, to land. You can do this by requesting beta uplift. For future reference, you should flag every patch that needs approval with sec-approval, but you only need to fill out the form once.

Attachment #9523928 - Flags: sec-approval? → sec-approval+

Updated

•

8 months ago

Attachment #9523927 - Flags: sec-approval? → sec-approval+

Updated

•

8 months ago

Attachment #9523929 - Flags: sec-approval? → sec-approval+

Updated

•

8 months ago

Attachment #9524366 - Flags: sec-approval? → sec-approval+

Updated

•

8 months ago

Attachment #9525288 - Flags: sec-approval? → sec-approval+

Updated

•

8 months ago

Attachment #9525570 - Flags: sec-approval? → sec-approval+

Assignee

Comment 50

•

8 months ago

•

Edited

Comment on attachment 9523928 [details]
(secure)

Beta/Release Uplift Approval Request

User impact if declined/Reason for urgency: This patchset fixes two branch offset calculation issues in 1) arm64 and 2) riscv64. If declined, some crafted JS code sequence may assemble jumps with truncated offset in release builds, disrupting the control flow.
Is this code covered by automated tests?: Yes
Has the fix been verified in Nightly?: No
Needs manual test from QE?: No
If yes, steps to reproduce: N/A.
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky):

The arm64 part is very small in code size.
The riscv64 part targets a Tier-3 architecture, and passes all local tests.
Changes to the shared code or other architectures are mostly no-ops that are unlikely to regress. Local tests are all good for x64, arm64 and loong64.

String changes made/needed: None.
Is Android affected?: Yes

Attachment #9523928 - Flags: approval-mozilla-beta?

Assignee

Updated

•

8 months ago

Attachment #9523927 - Flags: approval-mozilla-beta?

Assignee

Updated

•

8 months ago

Attachment #9523929 - Flags: approval-mozilla-beta?

Assignee

Updated

•

8 months ago

Attachment #9524366 - Flags: approval-mozilla-beta?

Assignee

Updated

•

8 months ago

Attachment #9525288 - Flags: approval-mozilla-beta?

Assignee

Updated

•

8 months ago

Attachment #9525570 - Flags: approval-mozilla-beta?

Comment 51

•

8 months ago

:npb, do you need to land this for :csmantle?
It will also need uplift requests for ESR115 and ESR140, looks like they will need a rebase.

Flags: needinfo?(nicolas.b.pierron)

Comment 52

•

8 months ago

(In reply to Rong Bao [:csmantle] from comment #42)

Comment on attachment 9523928 [details]
(secure)

Security Approval Request

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No

I would be cautious with this change, as pointed out in comment 16.
This change content does paint a bulls-eye on the security issue.

The change adds a factor of 4 instead of an implicit 1, which is used in the follow-up comparison, which suggests to any attacker that they should find a way to increase the deadline counter faster than the threshold is reached to generate a branch which can do some random code execution of a previously JIT-compiled constant pool.

Assignee

Comment 53

•

8 months ago

I can squash Part 1 into Part 2 if that would be better.

Comment 54

•

8 months ago

What would you think about comment 52 & comment 53 ?

Flags: needinfo?(tom)

Comment 55

•

8 months ago

(In reply to Nicolas B. Pierron [:nbp] from comment #54)

What would you think about comment 52 & comment 53 ?

Thanks for flagging. This question is one of the inputs (along with other inputs like where in the cycle are we, what type of bug is, what is the impact of the bug) that would lead to an output decision like "Let's land this later in the cycle", "Let's fold this into an innocuous public patch", "Let's reword the commit message or code changes" or "Okay to land now". Given the other inputs (2 days before last uplifts, content process bug) I think this change is fine to land as-is right now.

Flags: needinfo?(tom)

Comment 56

•

8 months ago

•

Edited

(In reply to Rong Bao [:csmantle] from comment #53)

I can squash Part 1 into Part 2 if that would be better.

This would not be necessary.
To echo Tom, you can land as-is as long as this is before Friday.

(In reply to Donal Meehan [:dmeehan] from comment #51)

:npb, do you need to land this for :csmantle?
It will also need uplift requests for ESR115 and ESR140, looks like they will need a rebase.

He should have commit access, otherwise I will do it.

Rong, can you ask for approval for both ESR versions?

Flags: needinfo?(nicolas.b.pierron) → needinfo?(webmaster)

Assignee

Comment 57

•

8 months ago

•

Edited

(In reply to Nicolas B. Pierron [:nbp] from comment #56)

This would not be necessary.
To echo Tom, you can land as-is as long as this is before Friday.

Thanks.

He should have commit access, otherwise I will do it.

Sorry, I only have access to Try trees, so I'm glad to have your help here.

Rong, can you ask for approval for both ESR versions?

Sure.

Flags: needinfo?(webmaster)

Assignee

Comment 58

•

8 months ago

•

Edited

Comment on attachment 9523927 [details]
(secure)

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec:high bug.
User impact if declined: ARM64 would be impacted by the wrong branch padding calculation.
Fix Landed on Version: Not yet.
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky):
- ARM64 patches have passed all local tests.
- Most tier-3 code are not enabled on these ESRs.

Attachment #9523927 - Flags: approval-mozilla-esr140?

Attachment #9523927 - Flags: approval-mozilla-esr115?

Assignee

Updated

•

8 months ago

Attachment #9523928 - Flags: approval-mozilla-esr140?

Attachment #9523928 - Flags: approval-mozilla-esr115?

Assignee

Updated

•

8 months ago

Attachment #9523929 - Flags: approval-mozilla-esr140?

Attachment #9523929 - Flags: approval-mozilla-esr115?

Assignee

Updated

•

8 months ago

Attachment #9524366 - Flags: approval-mozilla-esr140?

Attachment #9524366 - Flags: approval-mozilla-esr115?

Assignee

Updated

•

8 months ago

Attachment #9525570 - Flags: approval-mozilla-esr140?

Attachment #9525570 - Flags: approval-mozilla-esr115?

Assignee

Updated

•

8 months ago

Attachment #9525288 - Flags: approval-mozilla-esr140?

Attachment #9525288 - Flags: approval-mozilla-esr115?

Comment 59

•

8 months ago

Pushed by npierron@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/36888c1cec97 https://hg.mozilla.org/integration/autoland/rev/3406c2ca9612 Part 1: (Drive-by) Append InstSize factor for secondaryVeneers. r=nbp https://github.com/mozilla-firefox/firefox/commit/2a4ecd4c3acc https://hg.mozilla.org/integration/autoland/rev/99fa19f9d73a [riscv64] Part 2: Pad branches to match veneer size. r=nbp https://github.com/mozilla-firefox/firefox/commit/9001885200b0 https://hg.mozilla.org/integration/autoland/rev/15be8ef11f9e [riscv64] Part 3: Use helpers in [Macro]Assembler. r=nbp https://github.com/mozilla-firefox/firefox/commit/bba78a6469c9 https://hg.mozilla.org/integration/autoland/rev/e9ac80186de9 [riscv64] Part 4: Supply good maxInsts in no-pool area size calculation. r=nbp https://github.com/mozilla-firefox/firefox/commit/fadf320e1749 https://hg.mozilla.org/integration/autoland/rev/2ed00a91bfea Part 5: (Drive-by) Update used archs in SMDOC for AssemblerBufferWithConstantPools. r=nbp https://github.com/mozilla-firefox/firefox/commit/f9cd51be543e https://hg.mozilla.org/integration/autoland/rev/85453929839d [riscv64] Part 6: (Drive-by) Improve debug output for branch linking. r=nbp https://github.com/mozilla-firefox/firefox/commit/e05257d0e2e7 https://hg.mozilla.org/integration/autoland/rev/93bd472344a3 apply code formatting via Lando

Comment 60

•

8 months ago

:csmantle and :nbp, these do not graft cleanly to ESR140/ESR115
They will need a rebase
https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift#Manual_Cherry-Pick_(Recommended_if_Conflicts_Are_Likely)

Flags: needinfo?(webmaster)

Flags: needinfo?(nicolas.b.pierron)

Assignee

Comment 61

•

8 months ago

Nicolas, can we only uplift Part 1 if later parts are causing conflicts? It seems that riscv64 JIT is not enabled on ESR140 (https://searchfox.org/firefox-esr140/rev/a7d25db0d19e3224f4f99cd6ef49b676b9d40fc0/js/moz.configure#238-251) and ESR115 (https://searchfox.org/firefox-esr115/rev/0369badbc0e260ed3ec2202452892adaf1b89de0/js/moz.configure#178-192).

Flags: needinfo?(webmaster)

Comment 62

•

8 months ago

(In reply to Rong Bao [:csmantle] from comment #61)

Nicolas, can we only uplift Part 1 if later parts are causing conflicts? It seems that riscv64 JIT is not enabled on ESR140 (https://searchfox.org/firefox-esr140/rev/a7d25db0d19e3224f4f99cd6ef49b676b9d40fc0/js/moz.configure#238-251) and ESR115 (https://searchfox.org/firefox-esr115/rev/0369badbc0e260ed3ec2202452892adaf1b89de0/js/moz.configure#178-192).

Yes, this is possible and the simplest way forward.
Let's do that.

Flags: needinfo?(nicolas.b.pierron)

Assignee

Comment 63

•

8 months ago

•

Edited

(In reply to Nicolas B. Pierron [:nbp] from comment #62)

Yes, this is possible and the simplest way forward.
Let's do that.

Thanks. I think my next step would be removing the ESR* uplift requests for other parts.

Assignee

Updated

•

8 months ago

Attachment #9523927 - Flags: approval-mozilla-esr140?

Attachment #9523927 - Flags: approval-mozilla-esr115?

Assignee

Updated

•

8 months ago

Attachment #9523929 - Flags: approval-mozilla-esr140?

Attachment #9523929 - Flags: approval-mozilla-esr115?

Assignee

Updated

•

8 months ago

Attachment #9524366 - Flags: approval-mozilla-esr140?

Attachment #9524366 - Flags: approval-mozilla-esr115?

Assignee

Updated

•

8 months ago

Attachment #9525288 - Flags: approval-mozilla-esr140?

Attachment #9525288 - Flags: approval-mozilla-esr115?

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Assignee

Updated

•

8 months ago

Attachment #9525570 - Flags: approval-mozilla-esr140?

Attachment #9525570 - Flags: approval-mozilla-esr115?

Comment 64

•

8 months ago

https://hg.mozilla.org/mozilla-central/rev/3406c2ca9612
https://hg.mozilla.org/mozilla-central/rev/99fa19f9d73a
https://hg.mozilla.org/mozilla-central/rev/15be8ef11f9e
https://hg.mozilla.org/mozilla-central/rev/e9ac80186de9
https://hg.mozilla.org/mozilla-central/rev/2ed00a91bfea
https://hg.mozilla.org/mozilla-central/rev/85453929839d

Group: javascript-core-security → core-security-release

Status: ASSIGNED → RESOLVED

Closed: 8 months ago

status-firefox147: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 147 Branch

Comment 65

•

8 months ago

Comment on attachment 9523927 [details]
(secure)

Approved for 146.0b9

Attachment #9523927 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 66

•

8 months ago

Comment on attachment 9523928 [details]
(secure)

Approved for 146.0b9

Attachment #9523928 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 67

•

8 months ago

Comment on attachment 9523929 [details]
(secure)

Approved for 146.0b9

Attachment #9523929 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 68

•

8 months ago

Comment on attachment 9524366 [details]
(secure)

Approved for 146.0b9

Attachment #9524366 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 69

•

8 months ago

Comment on attachment 9525288 [details]
(secure)

Approved for 146.0b9

Attachment #9525288 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 70

•

8 months ago

Comment on attachment 9525570 [details]
(secure)

Approved for 146.0b9

Attachment #9525570 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

•

8 months ago

status-firefox146: affected → fixed

https://github.com/mozilla-firefox/firefox/commit/78dc0254b6f1 https://hg.mozilla.org/releases/mozilla-beta/rev/402fa2d956c6 https://github.com/mozilla-firefox/firefox/commit/06aa0b61646a https://hg.mozilla.org/releases/mozilla-beta/rev/cfcd222846a1 https://github.com/mozilla-firefox/firefox/commit/dfb318eb3652 https://hg.mozilla.org/releases/mozilla-beta/rev/23075f31e4dd https://github.com/mozilla-firefox/firefox/commit/73087015f803 https://hg.mozilla.org/releases/mozilla-beta/rev/f8911da0d29a https://github.com/mozilla-firefox/firefox/commit/0da5c49f020f https://hg.mozilla.org/releases/mozilla-beta/rev/1223ae2945ea https://github.com/mozilla-firefox/firefox/commit/141b66109759 https://hg.mozilla.org/releases/mozilla-beta/rev/50ba3c66007c

Comment 71

•

8 months ago

uplift

Andrei Vaida [:avaida]

Updated

•

8 months ago

QA Whiteboard: [sec] [uplift] [qa-triage-done-c147/b146]

Comment 72

•

8 months ago

Comment on attachment 9523928 [details]
(secure)

Approved for 140.6esr

Attachment #9523928 - Flags: approval-mozilla-esr140? → approval-mozilla-esr140+

https://hg.mozilla.org/releases/mozilla-esr140/rev/082892bbde13

Comment 73

•

8 months ago

uplift

Updated

•

8 months ago

status-firefox-esr140: affected → fixed

Comment 74

•

8 months ago

Comment on attachment 9523928 [details]
(secure)

Approved for 115.31esr

Attachment #9523928 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+

Updated

•

8 months ago

status-firefox-esr115: affected → fixed

https://hg.mozilla.org/releases/mozilla-esr115/rev/9681349bd179

Comment 75

•

8 months ago

uplift

Simon Friedberger [:simonf]

Updated

•

8 months ago

Whiteboard: [adv-main146+]

Simon Friedberger [:simonf]

Updated

•

8 months ago

Whiteboard: [adv-main146+] → [adv-main146+][adv-esr115.31+]

Simon Friedberger [:simonf]

Updated

•

8 months ago

Whiteboard: [adv-main146+][adv-esr115.31+] → [adv-main146+][adv-esr115.31+][adv-esr140.6+]

Daniel Veditz [:dveditz]

Comment 76

•

8 months ago

We are awarding a bounty to cmantle primarily for the secondary bug they identified that affected ARM64. The original RISC64 bug does not qualify for our bounty program because it is an unsupported tier-3 platform, but was are also awarding a smaller bounty to the original reporter because it did lead to the discovery of the second bug.

Flags: sec-bounty? → sec-bounty+

Assignee

Updated

•

7 months ago

Blocks: 2002969