jameysharp added the bug label to Issue #8690.
jameysharp added the fuzz-bug label to Issue #8690.
jameysharp opened issue #8690:
OSS-Fuzz reported this as https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69209. I've minimized the test case and identified several related panics, all with the message "unimplemented for > 64 bits".
This is not a security issue according to Wasmtime's security policy because it's a panic at compile time. It also shouldn't be reachable from Wasmtime today, only other Cranelift frontends.
<details>
<summary>Minimized CLIF</summary>test compile set opt_level=speed target x86_64 function %slt() system_v { block0: v0 = iconst.i64 0 v1 = uextend.i128 v0 v2 = icmp slt v1, v1 return } function %ult() system_v { block0: v0 = iconst.i64 0 v1 = uextend.i128 v0 v2 = icmp ult v1, v1 return }
</details>
The original fuzzbug, and my
%slt
function above, hit this panic inty_smin
: https://github.com/bytecodealliance/wasmtime/blob/f40aaa51a1df835498985acfc91ade33240d02b5/cranelift/codegen/src/isle_prelude.rs#L287-L289I believe the optimization rule which invoked
ty_smin
is here: https://github.com/bytecodealliance/wasmtime/blob/f40aaa51a1df835498985acfc91ade33240d02b5/cranelift/codegen/src/opts/icmp.isle#L105-L108There are similar rules for
ult
exercised by my%ult
function above. They callty_umax
, which callsty_mask
, which has essentially the same panic.There are also similar rules for
slt
which would hit an equivalent panic inty_smax
, but I can't exercise them because ISLE is choosing to callty_smin
first which then panics.I believe these panics are only being exposed now because #8686 allowed these rules to match i128 constants when they couldn't before. The rules should still fail to match because of the
fits_in_64
constraint on the other type in the pattern. However, ISLE does not guarantee anything about what order constructors and extractors will be evaluated in except for data dependencies. So thatfits_in_64
guard isn't sufficient to ensure thatty_smin
won't be called.One fix that I've verified works for the
%slt
test case:
- add the
partial
keyword to(decl pure ty_smin ...)
and similarly forty_smax
,- make the corresponding Rust functions return
Option
,- use
?
instead of.expect()
when.checked_sub()
fails.However I ran out of energy when I looked at doing the same for
ty_umax
, which requires doing it forty_mask
, which is called from Rust in a lot of places.cc: @scottmcm
alexcrichton commented on issue #8690:
Would it perhaps make sense to revert https://github.com/bytecodealliance/wasmtime/pull/8686 while a "true fix" is found in the meantime?
scottmcm commented on issue #8690:
From my side, feel free to revert my PR for now. I likely won't be able to look at fixing this in the short term.
alexcrichton commented on issue #8690:
I've posted https://github.com/bytecodealliance/wasmtime/pull/8707 to revert the original PR and included this issue's test cases as well so a re-landing will be sure to trigger the new test case too.
alexcrichton closed issue #8690:
OSS-Fuzz reported this as https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69209. I've minimized the test case and identified several related panics, all with the message "unimplemented for > 64 bits".
This is not a security issue according to Wasmtime's security policy because it's a panic at compile time. It also shouldn't be reachable from Wasmtime today, only other Cranelift frontends.
<details>
<summary>Minimized CLIF</summary>test compile set opt_level=speed target x86_64 function %slt() system_v { block0: v0 = iconst.i64 0 v1 = uextend.i128 v0 v2 = icmp slt v1, v1 return } function %ult() system_v { block0: v0 = iconst.i64 0 v1 = uextend.i128 v0 v2 = icmp ult v1, v1 return }
</details>
The original fuzzbug, and my
%slt
function above, hit this panic inty_smin
: https://github.com/bytecodealliance/wasmtime/blob/f40aaa51a1df835498985acfc91ade33240d02b5/cranelift/codegen/src/isle_prelude.rs#L287-L289I believe the optimization rule which invoked
ty_smin
is here: https://github.com/bytecodealliance/wasmtime/blob/f40aaa51a1df835498985acfc91ade33240d02b5/cranelift/codegen/src/opts/icmp.isle#L105-L108There are similar rules for
ult
exercised by my%ult
function above. They callty_umax
, which callsty_mask
, which has essentially the same panic.There are also similar rules for
slt
which would hit an equivalent panic inty_smax
, but I can't exercise them because ISLE is choosing to callty_smin
first which then panics.I believe these panics are only being exposed now because #8686 allowed these rules to match i128 constants when they couldn't before. The rules should still fail to match because of the
fits_in_64
constraint on the other type in the pattern. However, ISLE does not guarantee anything about what order constructors and extractors will be evaluated in except for data dependencies. So thatfits_in_64
guard isn't sufficient to ensure thatty_smin
won't be called.One fix that I've verified works for the
%slt
test case:
- add the
partial
keyword to(decl pure ty_smin ...)
and similarly forty_smax
,- make the corresponding Rust functions return
Option
,- use
?
instead of.expect()
when.checked_sub()
fails.However I ran out of energy when I looked at doing the same for
ty_umax
, which requires doing it forty_mask
, which is called from Rust in a lot of places.cc: @scottmcm
Last updated: Dec 23 2024 at 13:07 UTC