wasmtime / issue #8690 cranelift: ty_smin/smax/umax/mask ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #8690 cranelift: ty_smin/smax/umax/mask ...

Wasmtime GitHub notifications bot (May 24 2024 at 23:22):

jameysharp added the bug label to Issue #8690.

Wasmtime GitHub notifications bot (May 24 2024 at 23:22):

jameysharp added the fuzz-bug label to Issue #8690.

Wasmtime GitHub notifications bot (May 24 2024 at 23:22):

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 in ty_smin: https://github.com/bytecodealliance/wasmtime/blob/f40aaa51a1df835498985acfc91ade33240d02b5/cranelift/codegen/src/isle_prelude.rs#L287-L289

I believe the optimization rule which invoked ty_smin is here: https://github.com/bytecodealliance/wasmtime/blob/f40aaa51a1df835498985acfc91ade33240d02b5/cranelift/codegen/src/opts/icmp.isle#L105-L108

There are similar rules for ult exercised by my %ult function above. They call ty_umax, which calls ty_mask, which has essentially the same panic.

There are also similar rules for slt which would hit an equivalent panic in ty_smax, but I can't exercise them because ISLE is choosing to call ty_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 that fits_in_64 guard isn't sufficient to ensure that ty_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 for ty_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 for ty_mask, which is called from Rust in a lot of places.

cc: @scottmcm

Wasmtime GitHub notifications bot (May 29 2024 at 15:51):

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?

Wasmtime GitHub notifications bot (May 29 2024 at 16:06):

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.

Wasmtime GitHub notifications bot (May 29 2024 at 18:25):

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.

Wasmtime GitHub notifications bot (May 30 2024 at 19:54):

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 in ty_smin: https://github.com/bytecodealliance/wasmtime/blob/f40aaa51a1df835498985acfc91ade33240d02b5/cranelift/codegen/src/isle_prelude.rs#L287-L289

I believe the optimization rule which invoked ty_smin is here: https://github.com/bytecodealliance/wasmtime/blob/f40aaa51a1df835498985acfc91ade33240d02b5/cranelift/codegen/src/opts/icmp.isle#L105-L108

There are similar rules for ult exercised by my %ult function above. They call ty_umax, which calls ty_mask, which has essentially the same panic.

There are also similar rules for slt which would hit an equivalent panic in ty_smax, but I can't exercise them because ISLE is choosing to call ty_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 that fits_in_64 guard isn't sufficient to ensure that ty_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 for ty_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 for ty_mask, which is called from Rust in a lot of places.

cc: @scottmcm

Last updated: Nov 22 2024 at 16:03 UTC