wasmtime / PR #12333 Cranelift: x64: fix user-controlled ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #12333 Cranelift: x64: fix user-controlled ...

Wasmtime GitHub notifications bot (Jan 13 2026 at 17:49):

cfallin opened PR #12333 from cfallin:icmp-recursion to bytecodealliance:main:

We had a set of rules introduced in #11097 that attempted to optimize the case of testing the result of an icmp for a nonzero value. This allowed optimization of, for example, (((x == 0) == 0) == 0 ...) to a single level, either x == 0 or x != 0 depending on even/odd nesting depth.

Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the i32.eqz operator (for example) as well.

Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right.

This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway.

Note: this was reported as a security issue by @venkkatesh-sekar (thanks!); per our security policy, compilation DoS is not covered, so this is being fixed in a public PR. I will subsequently make a patch release to our currently supported versions (v36, v39, v40, 41).

Wasmtime GitHub notifications bot (Jan 13 2026 at 17:49):

cfallin requested fitzgen for a review on PR #12333.

Wasmtime GitHub notifications bot (Jan 13 2026 at 17:49):

cfallin requested wasmtime-compiler-reviewers for a review on PR #12333.

Wasmtime GitHub notifications bot (Jan 13 2026 at 17:49):

cfallin requested alexcrichton for a review on PR #12333.

Wasmtime GitHub notifications bot (Jan 13 2026 at 17:50):

cfallin edited PR #12333:

We had a set of rules introduced in #11097 that attempted to optimize the case of testing the result of an icmp for a nonzero value. This allowed optimization of, for example, (((x == 0) == 0) == 0 ...) to a single level, either x == 0 or x != 0 depending on even/odd nesting depth.

Unfortunately this kind of recursion in the backend has a depth bounded only by the user input, hence creates a compiler DoS vulnerability: the wrong kind of compiler input can cause a stack overflow in Cranelift at compilation time. This case is reachable from Wasmtime's Wasm frontend via the i32.eqz operator (for example) as well.

Ideally, this kind of deep rewrite is best done in our mid-end optimizer, where we think carefully about bounds for recursive rewrites. The left-hand sides for the backend rules should really be fixed shapes that correspond to machine instructions, rather than ad-hoc peephole optimizations in their own right.

This fix thus simply removes the recursion case that causes the blowup. The patch includes two tests: one with optimizations disabled, showing correct compilation (without the fix, this case fails to compile with a stack overflow), and one with optimizations enabled, showing that the mid-end properly cleans up the nested expression and we get the expected one-level result anyway.

Note: this was reported as a security issue by @venkkatesh-sekar (thanks!); per our security policy, compilation DoS is not covered, so this is being fixed in a public PR. I will subsequently make a patch release to our currently supported versions (v36, v39, v40, 41).

Wasmtime GitHub notifications bot (Jan 13 2026 at 17:50):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 13 2026 at 17:51):

cfallin has enabled auto merge for PR #12333.

Wasmtime GitHub notifications bot (Jan 13 2026 at 18:19):

alexcrichton commented on PR #12333:

I haven't dug deeply into the fallout here, but it looks like just removing these rules isn't enough to retain the codegen we have from before. A rough hunch is that there's a number of entrypoints for using these ISLE helpers and while the wasm test case here is one which is also cleaned up by the optimizer I think that these rules might be load-bearing for other codegen patterns unrelated to the optimizer.

It might need to be the case that some entrypoints use one rule, which includes these deleted rules, but then these rules don't recurse on themselves?

Wasmtime GitHub notifications bot (Jan 13 2026 at 18:21):

alexcrichton commented on PR #12333:

Although, from another angle, this seems fine as a thing to backport since it's clearly such low impact. I'm not aware of these codegen patterns being load bearing in terms of performance so a temporary minor regression while a "full fix" is developed for main I think would also be reasonable. In such a case though there's a number of codegen/etc tests to update to get CI passing.

Wasmtime GitHub notifications bot (Jan 13 2026 at 18:59):

cfallin commented on PR #12333:

This broke some existing lowerings; playing a bit more with various ways to break the cycle.

Wasmtime GitHub notifications bot (Jan 13 2026 at 19:00):

cfallin commented on PR #12333:

(On refresh, what Alex said -- somehow those comments didn't appear for me.)

Wasmtime GitHub notifications bot (Jan 13 2026 at 20:33):

cfallin updated PR #12333.

Wasmtime GitHub notifications bot (Jan 13 2026 at 20:35):

cfallin commented on PR #12333:

OK, I've reworked things a bit to preserve exactly the same codegen on branches -- I effectively peeled one layer off of is_nonzero_cmp to continue to use with branches but the previous backedge from emit_cmp to is_nonzero_cmp now invokes is_nonzero which has only the base cases. Together with explicit LHS patterns to peel one level of uextend this leaves us with no test-output changes now.

Wasmtime GitHub notifications bot (Jan 13 2026 at 20:37):

cfallin updated PR #12333.

Wasmtime GitHub notifications bot (Jan 13 2026 at 20:37):

cfallin requested wasmtime-core-reviewers for a review on PR #12333.

Wasmtime GitHub notifications bot (Jan 13 2026 at 20:37):

cfallin commented on PR #12333:

Or, well, spoke too soon -- no Cranelift filetest changes. A few disas tests that do FP compares do regress to materializing the bool.

Wasmtime GitHub notifications bot (Jan 13 2026 at 21:55):

cfallin updated PR #12333.

Wasmtime GitHub notifications bot (Jan 13 2026 at 21:58):

cfallin commented on PR #12333:

OK, I've gotten full equivalence on the disas tests too -- this needed one new mid-end rule to see through a pattern (fcmp(...) != 0) that we were previously only ever simplifying with this peephole rule in the backend.

@fitzgen mind giving this another look, since it's a little heavier than before?

Re: backport, I could certainly backport the rule deletions only, but I'd be a little concerned about performance regressions in that case because this affects many branches. If we feel ok about these rewrites I think the full patch should be applicable back to v36 (LTS).

Wasmtime GitHub notifications bot (Jan 13 2026 at 22:42):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 13 2026 at 22:42):

alexcrichton created PR review comment:

I think this change, the uextend-no-longer-recurses, is what's causing the remaining disassembly differences. Locally I'm seeing (uextend (vall_{any,true} ..)) I believe for example.

Do you think it'd be worth adding those specifically here as well? I'm not sure how to deduplicate this amongst the is_nonzero selectors above otherwise. Although nowadays egraphs might be able to rewrite (brif (uextend val)) to (brif val) which I know historically was difficult

Wasmtime GitHub notifications bot (Jan 13 2026 at 22:44):

alexcrichton commented on PR #12333:

Also, logistically, @cfallin are you ok running point on backporting/point releases? And 24.0.0, while supported, is unaffected right?

Wasmtime GitHub notifications bot (Jan 13 2026 at 22:50):

cfallin commented on PR #12333:

Yes, I'm happy to do the backports too. (Sorry, very slow today taking scraps of time between meetings and other interrupts; can hopefully do backports by end of tomorrow at latest.)

Wasmtime GitHub notifications bot (Jan 13 2026 at 22:53):

alexcrichton commented on PR #12333:

Oh no worries! Just want to make sure there's explicitly an owner. Tomorrow is totally fine and I can be around to hit approvals and help babysit CI. We've had some CI changes recently like trusted publishing which raises the risk of broken CI on other branches, so something to watch out for.

Wasmtime GitHub notifications bot (Jan 13 2026 at 22:59):

cfallin updated PR #12333.

Wasmtime GitHub notifications bot (Jan 13 2026 at 22:59):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 13 2026 at 22:59):

cfallin created PR review comment:

Ah, I missed this -- fixed in latest push (which drops the commit with any diffs to tests at all).

Wasmtime GitHub notifications bot (Jan 13 2026 at 22:59):

cfallin created PR review comment:

(Fixed by adding explicit patterns in is_nonzero as you suggest)

Wasmtime GitHub notifications bot (Jan 13 2026 at 22:59):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 13 2026 at 23:00):

cfallin commented on PR #12333:

OK, this should be good to go -- if r+ here then I can put up backports momentarily.

Wasmtime GitHub notifications bot (Jan 13 2026 at 23:00):

cfallin edited a comment on PR #12333:

OK, this should be good to go -- if r+ here (on new changes) then I can put up backports momentarily.

Wasmtime GitHub notifications bot (Jan 13 2026 at 23:10):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 13 2026 at 23:11):

cfallin has enabled auto merge for PR #12333.

Wasmtime GitHub notifications bot (Jan 13 2026 at 23:36):

cfallin merged PR #12333.

Wasmtime GitHub notifications bot (Jan 14 2026 at 10:46):

adambratschikaye commented on PR #12333:

Thanks for taking care of this @cfallin, @alexcrichton, and @fitzgen!

Last updated: Jan 29 2026 at 13:25 UTC