Closed Bug 1582727 Opened 5 months ago Closed 5 months ago

Interrupt checks confuse stack limit checks for functions with large stack frames

Categories

(Core :: Javascript: WebAssembly, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

Details

Attachments

(1 file)

Thanks decoder for sending quite an obscure bug my way :). From looking at the code a while in RR and trying to understand what happened, I could then craft a test case, attached in incoming patch.

When we set an interrupt, it sets TlsData::stackLimit to uintptr max. A stack limit check is emitted as follows for Ion/Cranelift:

  • put SP in a temp register
  • subtract Tls->stackLimit from temp (so temp := SP - stack limit, which is guaranteed to be positive since we maintain the invariant that SP >= stack limit, in a world where allocating stack memory means subtracting from SP)
  • if temp >(signed) allocation size, that is, if SP - stack limit > allocation size, we're all good. Otherwise, we generate a software trap.

Now, when the interrupt flag is set, TlsData::stackLimit is set to uintptr max, so SP - stack limit === SP + 1, and this flag is never honored. It means that both the interrupt check is never done, and that the stack overflow check is meaningless.

The baseline compiler isn't affected, because it does something different:

  • subtract amount from SP
  • then check if the resulting SP >(unsigned) stack limit.

(There might be a cleanup to do there: the stack allocation for Ion/Cranelift mentions that doing so might result in a borked SP value when we end up in the signal handler.)

I think Ion/Cranelift want the same as baseline (still using the temp). If we want to make sure that SP - amount doesn't overflow, we'd need a supplementary check; but it doesn't seem likely we'd ever need it.

(decoder told me this isn't sec-sensitive on IRC)

Oof, nice job tracking this down. Sounds like we can't rely on the invariant that SP >= stackLimit.

Some notes about the way baseline compiler handles this. There might or might not be bugs, not sure.

First, let's state the four roles of the stack limit check:

  1. Make sure that (sp - subtrahend) don't cause an unsigned overflow.
  2. Make sure that (sp - subtrahend) > stackLimit.
  3. Don't pass a borked SP to the trap handler, when the (2) check failed.
  4. Make sure that when stackLimit is 0xFFF...FFF, this causes a trap (that will be redirected to the interrupt check handler).

The baseline compiler handles both 2 and 4 properly with an explicit branch.

1 relies on the fact that the max subtrahend in the baseline compiler is 512Kb and that the only way to overflow it would be that the SP value is in the low 512 KB range of virtual memory addresses. There's nothing that enforces this as the moment.

I don't know what makes 3 works in practice: perhaps since the maximal subtrahend is 512 Kb, there's still some room for stack storage even when sp - subtrahend < stackLimit.

Ion/Cranelift can do unbounded stack allocations, and that's why they have to explicitly handle 1 and 3, is my current understanding of the situation.

OSes tend to put user space in the low half of memory, and furthermore make the lowest pages inaccessible. So one question is: how big is the usual inaccessible region: just 4kb, or something bigger? If it was big enough, maybe we could limit both baseline/ion/cranelift frame sizes to that and thereby not have to worry about underflow.

My reasoning was that OSes also allow stacks to grow fairly large and stacks always grow down (by SpiderMonkey assumption); hence they will strongly tend to be allocated high up in user memory. I should have added code somewhere to check that assumption at runtime, but even so I think I was right :)

We could probably get away with this by adding a release assert somewhere that checks the stack pointer and ensures it is not in the low part of memory, for "low" something like "4 * max(8MB, chosen stack size)" say, but even outlawing the low 512MB would probably work just fine for platforms that can reasonably run Firefox. At least with a check like that we're sure that a static frame limit of, say, 512KB is fine so far as overflow / trap behavior is concerned. We could probably also simplify the Ion test for stack frames up to that size, or to a size like that.

The "512KB" in the baseline compiler may depend on the stack headroom being at least that large (ideally a bit larger), for the trap handler to end up with a safe SP. It would be useful to assert that that is always true too.

In the worst case, in the baseline compiler, we could add back the overflowing offset after the branch, before the trap. It would add to code size but should not cause a performance regression. We'd still want to depend on the subtract not wrapping around, to avoid more complicated code.

Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a2a1dcc0931
wasm: tweak stack-limit comparison so it also works for interrupts; r=luke
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.