Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiled Module loading should check isa flags against host cpu #3897

Closed
pchickey opened this issue Mar 8, 2022 · 2 comments · Fixed by #3899
Closed

Compiled Module loading should check isa flags against host cpu #3897

pchickey opened this issue Mar 8, 2022 · 2 comments · Fixed by #3899
Assignees

Comments

@pchickey
Copy link
Contributor

pchickey commented Mar 8, 2022

Loading a compiled module into wasmtime may mean the Module was compiled on a different machine. Wasmtime does not provide a mechanism to make sure the set of flags on a compiled module is compatible with the CPU that the compiled module will be executed on.

Presently, when wasmtime loads a compiled Module (e.g. Module::deserialize_file), the isa flags in the loaded module are only checked when cfg(compiler):

#[cfg(compiler)]
{
let compiler = engine.compiler();
self.check_shared_flags(compiler)?;
self.check_isa_flags(compiler)?;
}

This check doesn't even perform precisely what we want - it will determine if the Engine's flags are compatible with the loaded module, which do not necessarily match the host cpu.

So, I propose the following additions to wasmtime:

  1. wasmtime::Config should have the target and cranelift_flag_set methods available even when the feature cranelift is not enabled (e.g. in runtime-only mode).
  2. wasmtime::Config should have a setting which mandates that Engine construction validates those target and cranelift flags against the host cpu. This needs to be configable so that an engine Engine can be used for cross-compilation.
  3. When loading a compiled Module, the isa flags should be checked against the Engine settings even when cfg(compiler) is not enabled.
@pchickey pchickey assigned cfallin and alexcrichton and unassigned cfallin Mar 8, 2022
@akirilov-arm
Copy link
Contributor

Concerning item 2, there is a subtlety - it might be acceptable to have some discrepancy between the features supported by the host CPU and those used by the compiled module if the difference consists of features that are backward-compatible. Examples are Pointer Authentication and Branch Target Identification from the Arm instruction set architecture (which I am discussing in bytecodealliance/rfcs#17 and have prototyped in PR #3693) and I believe that the ENDBR instruction introduced by Intel CET behaves similarly.

cc @abrown

alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Mar 8, 2022
This commit fills out a few FIXME annotations by implementing run-time
checks that when a `Module` is created it has compatible codegen
settings for the current host (as `Module` is proof of "this code can
run"). This is done by implementing new `Engine`-level methods which
validate compiler settings. These settings are validated on
`Module::new` as well as when loading serialized modules.

Settings are split into two categories, one for "shared" top-level
settings and one for ISA-specific settings. Both categories now have
allow-lists hardcoded into `Engine` which indicate the acceptable values
for each setting (if applicable). ISA-specific settings are checked with
the Rust standard library's `std::is_x86_feature_detected!` macro. Other
macros for other platforms are not stable at this time but can be added
here if necessary.

Closes bytecodealliance#3897
@alexcrichton
Copy link
Member

I implemented #3899 in a way that can accomodate the pointer authentication bits since the query made is "is this feature compatible with the native host" so there's room to say whether or not pointer authentication is enabled that's always compatible with the native host (possibly with extra handling to enter/exit the module in a different fashion if it's enabled/disabled).

alexcrichton added a commit that referenced this issue Mar 9, 2022
* Implement runtime checks for compilation settings

This commit fills out a few FIXME annotations by implementing run-time
checks that when a `Module` is created it has compatible codegen
settings for the current host (as `Module` is proof of "this code can
run"). This is done by implementing new `Engine`-level methods which
validate compiler settings. These settings are validated on
`Module::new` as well as when loading serialized modules.

Settings are split into two categories, one for "shared" top-level
settings and one for ISA-specific settings. Both categories now have
allow-lists hardcoded into `Engine` which indicate the acceptable values
for each setting (if applicable). ISA-specific settings are checked with
the Rust standard library's `std::is_x86_feature_detected!` macro. Other
macros for other platforms are not stable at this time but can be added
here if necessary.

Closes #3897

* Fix fall-through logic to actually be correct

* Use a `OnceCell`, not an `AtomicBool`

* Fix some broken tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants