Fix a soundness issue with lowering variants (#4723)

* Fix a compile error on nightly Rust

It looks like Rust nightly has gotten a bit more strict about
attributes-on-expressions and previously accepted code is no longer
accepted. This commit updates the generated code for a macro to a form
which is accepted by rustc.

* Fix a soundness issue with lowering variants

This commit fixes a soundness issue lowering variants in the component
model where host memory could be leaked to the guest module by accident.
In reviewing code recently for `Val::lower` I noticed that the variant
lowering was extending the payload with `ValRaw::u32(0)` to
appropriately fit the size of the variant. In reading this it appeared
incorrect to me due to the fact that it should be `ValRaw::u64(0)` since
up to 64-bits can be read. Additionally this implementation was also
incorrect because the lowered representation of the payload itself was
not possibly zero-extended to 64-bits to accommodate other variants.

It turned out these issues were benign because with the dynamic
surface area to the component model the arguments were all initialized
to 0 anyway. The static version of the API, however, does not initialize
arguments to 0 and I wanted to initially align these two implementations
so I updated the variant implementation of lowering for dynamic values
and removed the zero-ing of arguments.

To test this change I updated the `debug` mode of adapter module
generation to assert that the upper bits of values in wasm are always
zero when the value is casted down (during `stack_get` which only
happens with variants). I then threaded through the `debug` boolean
configuration parameter into the dynamic and static fuzzers.

To my surprise this new assertion tripped even after the fix was
applied. It turns out, though, that there was other leakage of bits
through other means that I was previously unaware of. At the primitive
level lowerings of types like `u32` will have a `Lower` representation
of `ValRaw` and the lowering is simply `dst.write(ValRaw::i32(self))`,
or the equivalent thereof. The problem, that the fuzzers detected, with
this pattern is that the `ValRaw` type is 16-bytes, and
`ValRaw::i32(X)` only initializes the first 4. This meant that all the
lowerings for all primitives were writing up to 12 bytes of garbage from
the host for the wasm module to read.

It turned out that this write of a `ValRaw` was sometimes 16 bytes and
sometimes the appropriate size depending on the number of optimizations
in play. With enough inlining for example `dst.write(ValRaw::i32(self))`
would only write 4 bytes, as expected. In debug mode though without
inlining 16 bytes would be written, including the garbage from the upper
bits.

To solve this issue I ended up taking a somewhat different approach. I
primarily updated the `ValRaw` constructors to simply always extend the
values internally to 64-bits, meaning that the low 8 bytes of a `ValRaw`
is always initialized. This prevents any undefined data from leaking
from the host into a wasm module, and means that values are also
zero-extended even if they're only used in 32-bit contexts outside of a
variant. This felt like the best fix for now, though, in terms of
not really having a performance impact while additionally not requiring
a rewrite of all lowerings.

This solution ended up also neatly removing the "zero out the entire
payload" logic that was previously require. Now after a payload is
lowered only the tail end of the payload, up to the size of the variant,
is zeroed out. This means that each lowered argument is written to at
most once which should hopefully be a small performance boost for
calling into functions as well.
This commit is contained in:
Alex Crichton
2022-08-16 17:33:24 -05:00
committed by GitHub
parent 83e37f9334
commit 5add267b87
11 changed files with 211 additions and 77 deletions

View File

@@ -618,7 +618,7 @@ impl Expander for LowerExpander {
if ty.is_some() {
pattern = quote!(Self::#ident(value));
lower = quote!(value.lower(store, options, #internal::map_maybe_uninit!(dst.payload.#ident)));
lower = quote!(value.lower(store, options, dst));
store = quote!(value.store(
memory,
offset + <Self as #internal::ComponentVariant>::PAYLOAD_OFFSET32,
@@ -630,8 +630,14 @@ impl Expander for LowerExpander {
}
lowers.extend(quote!(#pattern => {
#internal::map_maybe_uninit!(dst.tag).write(wasmtime::ValRaw::i32(#index_u32 as i32));
#lower
#internal::map_maybe_uninit!(dst.tag).write(wasmtime::ValRaw::u32(#index_u32));
unsafe {
#internal::lower_payload(
#internal::map_maybe_uninit!(dst.payload),
|payload| #internal::map_maybe_uninit!(payload.#ident),
|dst| #lower,
)
}
}));
stores.extend(quote!(#pattern => {
@@ -652,13 +658,6 @@ impl Expander for LowerExpander {
options: &#internal::Options,
dst: &mut std::mem::MaybeUninit<Self::Lower>,
) -> #internal::anyhow::Result<()> {
// See comment in <Result<T, E> as Lower>::lower for why we zero out the payload here
unsafe {
#internal::map_maybe_uninit!(dst.payload)
.as_mut_ptr()
.write_bytes(0u8, 1);
}
match self {
#lowers
}
@@ -791,13 +790,10 @@ impl Expander for ComponentTypeExpander {
}
VariantStyle::Enum => quote!(#name,),
});
lower_payload_case_declarations.extend(quote!(#ident: [wasmtime::ValRaw; 0],));
}
}
if lower_payload_case_declarations.is_empty() {
lower_payload_case_declarations.extend(quote!(_dummy: ()));
}
let typecheck = match style {
VariantStyle::Variant => quote!(typecheck_variant),
VariantStyle::Union => quote!(typecheck_union),

View File

@@ -2485,10 +2485,13 @@ impl Compiler<'_, '_> {
| (ValType::F64, ValType::F64) => {}
(ValType::I32, ValType::F32) => self.instruction(F32ReinterpretI32),
(ValType::I64, ValType::I32) => self.instruction(I32WrapI64),
(ValType::I64, ValType::I32) => {
self.assert_i64_upper_bits_not_set(idx);
self.instruction(I32WrapI64);
}
(ValType::I64, ValType::F64) => self.instruction(F64ReinterpretI64),
(ValType::F64, ValType::F32) => self.instruction(F32DemoteF64),
(ValType::I64, ValType::F32) => {
self.assert_i64_upper_bits_not_set(idx);
self.instruction(I32WrapI64);
self.instruction(F32ReinterpretI32);
}
@@ -2501,6 +2504,7 @@ impl Compiler<'_, '_> {
| (ValType::F32, ValType::F64)
| (ValType::F64, ValType::I32)
| (ValType::F64, ValType::I64)
| (ValType::F64, ValType::F32)
// not used in the component model
| (ValType::ExternRef, _)
@@ -2514,6 +2518,19 @@ impl Compiler<'_, '_> {
}
}
fn assert_i64_upper_bits_not_set(&mut self, local: u32) {
if !self.module.debug {
return;
}
self.instruction(LocalGet(local));
self.instruction(I64Const(32));
self.instruction(I64ShrU);
self.instruction(I32WrapI64);
self.instruction(If(BlockType::Empty));
self.trap(Trap::AssertFailed("upper bits are unexpectedly set"));
self.instruction(End);
}
/// Converts the top value on the WebAssembly stack which has type
/// `src_ty` to `dst_tys[0]`.
///
@@ -2531,7 +2548,6 @@ impl Compiler<'_, '_> {
(ValType::F32, ValType::I32) => self.instruction(I32ReinterpretF32),
(ValType::I32, ValType::I64) => self.instruction(I64ExtendI32U),
(ValType::F64, ValType::I64) => self.instruction(I64ReinterpretF64),
(ValType::F32, ValType::F64) => self.instruction(F64PromoteF32),
(ValType::F32, ValType::I64) => {
self.instruction(I32ReinterpretF32);
self.instruction(I64ExtendI32U);
@@ -2545,6 +2561,7 @@ impl Compiler<'_, '_> {
| (ValType::F64, ValType::F32)
| (ValType::I32, ValType::F64)
| (ValType::I64, ValType::F64)
| (ValType::F32, ValType::F64)
// not used in the component model
| (ValType::ExternRef, _)

View File

@@ -141,6 +141,7 @@ macro_rules! define_static_api_test {
let mut config = Config::new();
config.wasm_component_model(true);
config.debug_adapter_modules(input.arbitrary()?);
let engine = Engine::new(&config).unwrap();
let wat = declarations.make_component();
let wat = wat.as_bytes();

View File

@@ -1087,7 +1087,9 @@ pub fn dynamic_component_api_target(input: &mut arbitrary::Unstructured) -> arbi
let case = input.arbitrary::<TestCase>()?;
let engine = component_test_util::engine();
let mut config = component_test_util::config();
config.debug_adapter_modules(input.arbitrary()?);
let engine = Engine::new(&config).unwrap();
let mut store = Store::new(&engine, (Box::new([]) as Box<[Val]>, None));
let wat = case.declarations().make_component();
let wat = wat.as_bytes();

View File

@@ -983,7 +983,13 @@ impl ValRaw {
/// Creates a WebAssembly `i32` value
#[inline]
pub fn i32(i: i32) -> ValRaw {
ValRaw { i32: i.to_le() }
// Note that this is intentionally not setting the `i32` field, instead
// setting the `i64` field with a zero-extended version of `i`. For more
// information on this see the comments on `Lower for Result` in the
// `wasmtime` crate. Otherwise though all `ValRaw` constructors are
// otherwise constrained to guarantee that the initial 64-bits are
// always initialized.
ValRaw::u64((i as u32).into())
}
/// Creates a WebAssembly `i64` value
@@ -995,7 +1001,9 @@ impl ValRaw {
/// Creates a WebAssembly `i32` value
#[inline]
pub fn u32(i: u32) -> ValRaw {
ValRaw::i32(i as i32)
// See comments in `ValRaw::i32` for why this is setting the upper
// 32-bits as well.
ValRaw::u64(i.into())
}
/// Creates a WebAssembly `i64` value
@@ -1007,7 +1015,9 @@ impl ValRaw {
/// Creates a WebAssembly `f32` value
#[inline]
pub fn f32(i: u32) -> ValRaw {
ValRaw { f32: i.to_le() }
// See comments in `ValRaw::i32` for why this is setting the upper
// 32-bits as well.
ValRaw::u64(i.into())
}
/// Creates a WebAssembly `f64` value

View File

@@ -1,4 +1,5 @@
use crate::component::instance::{Instance, InstanceData};
use crate::component::storage::storage_as_slice;
use crate::component::types::Type;
use crate::component::values::Val;
use crate::store::{StoreOpaque, Stored};
@@ -317,8 +318,6 @@ impl Func {
if param_abi.flat_count(MAX_FLAT_PARAMS).is_none() {
self.store_args(store, &options, &param_abi, &params, args, dst)
} else {
dst.write([ValRaw::u64(0); MAX_FLAT_PARAMS]);
let dst = &mut unsafe {
mem::transmute::<_, &mut [MaybeUninit<ValRaw>; MAX_FLAT_PARAMS]>(dst)
}
@@ -443,7 +442,7 @@ impl Func {
// later get used in post-return.
flags.set_needs_post_return(true);
let val = lift(store.0, &options, ret)?;
let ret_slice = cast_storage(ret);
let ret_slice = storage_as_slice(ret);
let data = &mut store.0[self.0];
assert!(data.post_return_arg.is_none());
match ret_slice.len() {
@@ -453,16 +452,6 @@ impl Func {
}
return Ok(val);
}
unsafe fn cast_storage<T>(storage: &T) -> &[ValRaw] {
assert!(std::mem::size_of_val(storage) % std::mem::size_of::<ValRaw>() == 0);
assert!(std::mem::align_of_val(storage) == std::mem::align_of::<ValRaw>());
std::slice::from_raw_parts(
(storage as *const T).cast(),
mem::size_of_val(storage) / mem::size_of::<ValRaw>(),
)
}
}
/// Invokes the `post-return` canonical ABI option, if specified, after a

View File

@@ -1,4 +1,5 @@
use crate::component::func::{Memory, MemoryMut, Options};
use crate::component::storage::slice_to_storage_mut;
use crate::component::{ComponentParams, ComponentType, Lift, Lower, Type, Val};
use crate::{AsContextMut, StoreContextMut, ValRaw};
use anyhow::{anyhow, bail, Context, Result};
@@ -197,7 +198,7 @@ where
// There's a 2x2 matrix of whether parameters and results are stored on the
// stack or on the heap. Each of the 4 branches here have a different
// representation of the storage of arguments/returns which is represented
// by the type parameter that we pass to `cast_storage`.
// by the type parameter that we pass to `slice_to_storage_mut`.
//
// Also note that while four branches are listed here only one is taken for
// any particular `Params` and `Return` combination. This should be
@@ -206,13 +207,15 @@ where
// branch, but today is not that day.
if Params::flatten_count() <= MAX_FLAT_PARAMS {
if Return::flatten_count() <= MAX_FLAT_RESULTS {
let storage = cast_storage::<ReturnStack<Params::Lower, Return::Lower>>(storage);
let storage =
slice_to_storage_mut::<ReturnStack<Params::Lower, Return::Lower>>(storage);
let params = Params::lift(cx.0, &options, &storage.assume_init_ref().args)?;
let ret = closure(cx.as_context_mut(), params)?;
flags.set_may_leave(false);
ret.lower(&mut cx, &options, map_maybe_uninit!(storage.ret))?;
} else {
let storage = cast_storage::<ReturnPointer<Params::Lower>>(storage).assume_init_ref();
let storage =
slice_to_storage_mut::<ReturnPointer<Params::Lower>>(storage).assume_init_ref();
let params = Params::lift(cx.0, &options, &storage.args)?;
let ret = closure(cx.as_context_mut(), params)?;
let mut memory = MemoryMut::new(cx.as_context_mut(), &options);
@@ -223,7 +226,7 @@ where
} else {
let memory = Memory::new(cx.0, &options);
if Return::flatten_count() <= MAX_FLAT_RESULTS {
let storage = cast_storage::<ReturnStack<ValRaw, Return::Lower>>(storage);
let storage = slice_to_storage_mut::<ReturnStack<ValRaw, Return::Lower>>(storage);
let ptr =
validate_inbounds::<Params>(memory.as_slice(), &storage.assume_init_ref().args)?;
let params = Params::load(&memory, &memory.as_slice()[ptr..][..Params::SIZE32])?;
@@ -231,7 +234,7 @@ where
flags.set_may_leave(false);
ret.lower(&mut cx, &options, map_maybe_uninit!(storage.ret))?;
} else {
let storage = cast_storage::<ReturnPointer<ValRaw>>(storage).assume_init_ref();
let storage = slice_to_storage_mut::<ReturnPointer<ValRaw>>(storage).assume_init_ref();
let ptr = validate_inbounds::<Params>(memory.as_slice(), &storage.args)?;
let params = Params::load(&memory, &memory.as_slice()[ptr..][..Params::SIZE32])?;
let ret = closure(cx.as_context_mut(), params)?;
@@ -263,22 +266,6 @@ fn validate_inbounds<T: ComponentType>(memory: &[u8], ptr: &ValRaw) -> Result<us
Ok(ptr)
}
unsafe fn cast_storage<T>(storage: &mut [ValRaw]) -> &mut MaybeUninit<T> {
// Assertions that LLVM can easily optimize away but are sanity checks here
assert!(std::mem::size_of::<T>() % std::mem::size_of::<ValRaw>() == 0);
assert!(std::mem::align_of::<T>() == std::mem::align_of::<ValRaw>());
assert!(std::mem::align_of_val(storage) == std::mem::align_of::<T>());
// This is an actual runtime assertion which if performance calls for we may
// need to relax to a debug assertion. This notably tries to ensure that we
// stay within the bounds of the number of actual values given rather than
// reading past the end of an array. This shouldn't actually trip unless
// there's a bug in Wasmtime though.
assert!(std::mem::size_of_val(storage) >= std::mem::size_of::<T>());
&mut *storage.as_mut_ptr().cast()
}
unsafe fn handle_result(func: impl FnOnce() -> Result<()>) {
match panic::catch_unwind(AssertUnwindSafe(func)) {
Ok(Ok(())) => {}

View File

@@ -1,4 +1,5 @@
use crate::component::func::{Func, Memory, MemoryMut, Options};
use crate::component::storage::{storage_as_slice, storage_as_slice_mut};
use crate::store::StoreOpaque;
use crate::{AsContext, AsContextMut, StoreContext, StoreContextMut, ValRaw};
use anyhow::{anyhow, bail, Context, Result};
@@ -1681,6 +1682,34 @@ where
}
}
/// Lowers the payload of a variant into the storage for the entire payload,
/// handling writing zeros at the end of the representation if this payload is
/// smaller than the entire flat representation.
///
/// * `payload` - the flat storage space for the entire payload of the variant
/// * `typed_payload` - projection from the payload storage space to the
/// individaul storage space for this variant.
/// * `lower` - lowering operation used to initialize the `typed_payload` return
/// value.
///
/// For more information on this se the comments in the `Lower for Result`
/// implementation below.
pub unsafe fn lower_payload<P, T>(
payload: &mut MaybeUninit<P>,
typed_payload: impl FnOnce(&mut MaybeUninit<P>) -> &mut MaybeUninit<T>,
lower: impl FnOnce(&mut MaybeUninit<T>) -> Result<()>,
) -> Result<()> {
let typed = typed_payload(payload);
lower(typed)?;
let typed_len = storage_as_slice(typed).len();
let payload = storage_as_slice_mut(payload);
for slot in payload[typed_len..].iter_mut() {
*slot = ValRaw::u64(0);
}
Ok(())
}
unsafe impl<T, E> ComponentVariant for Result<T, E>
where
T: ComponentType,
@@ -1700,35 +1729,89 @@ where
options: &Options,
dst: &mut MaybeUninit<Self::Lower>,
) -> Result<()> {
// Start out by zeroing out the payload. This will ensure that if either
// arm doesn't initialize some values then everything is still
// deterministically set.
// This implementation of `Lower::lower`, if you're reading these from
// the top of this file, is the first location that the "join" logic of
// the component model's canonical ABI encountered. The rough problem is
// that let's say we have a component model type of the form:
//
// Additionally, this initialization of zero means that the specific
// types written by each `lower` call below on each arm still has the
// correct value even when "joined" with the other arm.
// (result u64 (error (tuple f32 u16)))
//
// Finally note that this is required by the canonical ABI to some
// degree where if the `Ok` arm initializes fewer values than the `Err`
// arm then all the remaining values must be initialized to zero, and
// that's what this does.
unsafe {
map_maybe_uninit!(dst.payload)
.as_mut_ptr()
.write_bytes(0u8, 1);
}
// The flat representation of this is actually pretty tricky. Currently
// it is:
//
// i32 i64 i32
//
// The first `i32` is the discriminant for the `result`, and the payload
// is represented by `i64 i32`. The "ok" variant will only use the `i64`
// and the "err" variant will use both `i64` and `i32`.
//
// In the "ok" variant the first issue is encountered. The size of one
// variant may not match the size of the other variants. All variants
// start at the "front" but when lowering a type we need to be sure to
// initialize the later variants (lest we leak random host memory into
// the guest module). Due to how the `Lower` type is represented as a
// `union` of all the variants what ends up happening here is that
// internally within the `lower_payload` after the typed payload is
// lowered the remaining bits of the payload that weren't initialized
// are all set to zero. This will guarantee that we'll write to all the
// slots for each variant.
//
// The "err" variant encounters the second issue, however, which is that
// the flat representation for each type may differ between payloads. In
// the "ok" arm an `i64` is written, but the `lower` implementation for
// the "err" arm will write an `f32` and then an `i32`. For this
// implementation of `lower` to be valid the `f32` needs to get inflated
// to an `i64` with zero-padding in the upper bits. What may be
// surprising, however, is that none of this is handled in this file.
// This implementation looks like it's blindly deferring to `E::lower`
// and hoping it does the right thing.
//
// In reality, however, the correctness of variant lowering relies on
// two subtle details of the `ValRaw` implementation in Wasmtime:
//
// 1. First the `ValRaw` value always contains little-endian values.
// This means that if a `u32` is written, a `u64` is read, and then
// the `u64` has its upper bits truncated the original value will
// always be retained. This is primarily here for big-endian
// platforms where if it weren't little endian then the opposite
// would occur and the wrong value would be read.
//
// 2. Second, and perhaps even more subtly, the `ValRaw` constructors
// for 32-bit types actually always initialize 64-bits of the
// `ValRaw`. In the component model flat ABI only 32 and 64-bit types
// are used so 64-bits is big enough to contain everything. This
// means that when a `ValRaw` is written into the destination it will
// always, whether it's needed or not, be "ready" to get extended up
// to 64-bits.
//
// Put together these two subtle guarantees means that all `Lower`
// implementations can be written "naturally" as one might naively
// expect. Variants will, on each arm, zero out remaining fields and all
// writes to the flat representation will automatically be 64-bit writes
// meaning that if the value is read as a 64-bit value, which isn't
// known at the time of the write, it'll still be correct.
match self {
Ok(e) => {
map_maybe_uninit!(dst.tag).write(ValRaw::i32(0));
e.lower(store, options, map_maybe_uninit!(dst.payload.ok))?;
unsafe {
lower_payload(
map_maybe_uninit!(dst.payload),
|payload| map_maybe_uninit!(payload.ok),
|dst| e.lower(store, options, dst),
)
}
}
Err(e) => {
map_maybe_uninit!(dst.tag).write(ValRaw::i32(1));
e.lower(store, options, map_maybe_uninit!(dst.payload.err))?;
unsafe {
lower_payload(
map_maybe_uninit!(dst.payload),
|payload| map_maybe_uninit!(payload.err),
|dst| e.lower(store, options, dst),
)
}
}
}
Ok(())
}
fn store<U>(&self, mem: &mut MemoryMut<'_, U>, offset: usize) -> Result<()> {

View File

@@ -8,6 +8,7 @@ mod func;
mod instance;
mod linker;
mod matching;
mod storage;
mod store;
pub mod types;
mod values;
@@ -28,8 +29,9 @@ pub use wasmtime_component_macro::{flags, ComponentType, Lift, Lower};
#[doc(hidden)]
pub mod __internal {
pub use super::func::{
format_flags, typecheck_enum, typecheck_flags, typecheck_record, typecheck_union,
typecheck_variant, ComponentVariant, MaybeUninitExt, Memory, MemoryMut, Options,
format_flags, lower_payload, typecheck_enum, typecheck_flags, typecheck_record,
typecheck_union, typecheck_variant, ComponentVariant, MaybeUninitExt, Memory, MemoryMut,
Options,
};
pub use crate::map_maybe_uninit;
pub use crate::store::StoreOpaque;

View File

@@ -0,0 +1,43 @@
use crate::ValRaw;
use std::mem::{self, MaybeUninit};
use std::slice;
fn assert_raw_slice_compat<T>() {
assert!(mem::size_of::<T>() % mem::size_of::<ValRaw>() == 0);
assert!(mem::align_of::<T>() == mem::align_of::<ValRaw>());
}
/// Converts a `<T as ComponentType>::Lower` representation to a slice of
/// `ValRaw`.
pub unsafe fn storage_as_slice<T>(storage: &T) -> &[ValRaw] {
assert_raw_slice_compat::<T>();
slice::from_raw_parts(
(storage as *const T).cast(),
mem::size_of_val(storage) / mem::size_of::<ValRaw>(),
)
}
/// Same as `storage_as_slice`, but mutable.
pub unsafe fn storage_as_slice_mut<T>(storage: &mut T) -> &mut [ValRaw] {
assert_raw_slice_compat::<T>();
slice::from_raw_parts_mut(
(storage as *mut T).cast(),
mem::size_of_val(storage) / mem::size_of::<ValRaw>(),
)
}
/// Same as `storage_as_slice`, but in reverse and mutable.
pub unsafe fn slice_to_storage_mut<T>(slice: &mut [ValRaw]) -> &mut MaybeUninit<T> {
assert_raw_slice_compat::<T>();
// This is an actual runtime assertion which if performance calls for we may
// need to relax to a debug assertion. This notably tries to ensure that we
// stay within the bounds of the number of actual values given rather than
// reading past the end of an array. This shouldn't actually trip unless
// there's a bug in Wasmtime though.
assert!(mem::size_of_val(slice) >= mem::size_of::<T>());
&mut *slice.as_mut_ptr().cast()
}

View File

@@ -875,10 +875,14 @@ impl Val {
}) => {
next_mut(dst).write(ValRaw::u32(*discriminant));
value.lower(store, options, dst)?;
// For the remaining lowered representation of this variant that
// the payload didn't write we write out zeros here to ensure
// the entire variant is written.
let value_flat = value.ty().canonical_abi().flat_count(usize::MAX).unwrap();
let variant_flat = self.ty().canonical_abi().flat_count(usize::MAX).unwrap();
for _ in (1 + value_flat)..variant_flat {
next_mut(dst).write(ValRaw::u32(0));
next_mut(dst).write(ValRaw::u64(0));
}
}
Val::Enum(Enum { discriminant, .. }) => {