Skip to content

Commit

Permalink
wiggle: adapt Wiggle guest slices for unsafe shared use
Browse files Browse the repository at this point in the history
When multiple threads can concurrently modify a WebAssembly shared
memory, the underlying data for a Wiggle `GuestSlice` and
`GuestSliceMut` could change due to access from other threads. This
breaks Rust guarantees when `&[T]` and `&mut [T]` slices are handed out.
This change modifies `GuestPtr` to make `as_slice` and `as_slice_mut`
return an `Option` which is `None` when the underlying WebAssembly
memory is shared.

But WASI implementations still need access to the underlying WebAssembly
memory, both to read to it and write from it. This change adds new APIs:
- `GuestPtr::to_vec` copies the  bytes from WebAssembly memory (from
  which we can safely take a `&[T]`)
- `GuestPtr::as_unsafe_slice_mut` returns a wrapper `struct` from which
  we can  `unsafe`-ly return a mutable slice (users must accept the
  unsafety of concurrently modifying a `&mut [T]`)

This approach allows us to maintain Wiggle's borrow-checking
infrastructure, which enforces the guarantee that Wiggle will not modify
overlapping regions, e.g. This is important because the underlying
system calls may expect this. Though other threads may modify the same
underlying region, this is impossible to prevent; at least Wiggle will
not be able to do so.

Finally, the changes to Wiggle's API are propagated to all WASI
implementations in Wasmtime. For now, code locations that attempt to get
a guest slice will panic if the underlying memory is shared. Note that
Wiggle is not enabled for shared memory (that will come later in
something like bytecodealliance#5054), but when it is, these panics will be clear
indicators of locations that must be re-implemented in a thread-safe
way.
  • Loading branch information
abrown committed Nov 8, 2022
1 parent f026d95 commit 85cc04d
Show file tree
Hide file tree
Showing 12 changed files with 363 additions and 118 deletions.
1 change: 0 additions & 1 deletion crates/wasi-common/src/dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,6 @@ pub(crate) trait TableDirExt {
fn get_dir(&self, fd: u32) -> Result<&DirEntry, Error>;
fn is_preopen(&self, fd: u32) -> bool;
}

impl TableDirExt for crate::table::Table {
fn get_dir(&self, fd: u32) -> Result<&DirEntry, Error> {
self.get(fd)
Expand Down
48 changes: 30 additions & 18 deletions crates/wasi-common/src/snapshots/preview_0.rs
Original file line number Diff line number Diff line change
Expand Up @@ -468,14 +468,16 @@ impl wasi_unstable::WasiUnstable for WasiCtx {
.get_file_mut(u32::from(fd))?
.get_cap_mut(FileCaps::READ)?;

let mut guest_slices: Vec<wiggle::GuestSliceMut<u8>> = iovs
.iter()
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Iovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice_mut()?)
})
.collect::<Result<_, Error>>()?;
let mut guest_slices: Vec<wiggle::GuestSliceMut<u8>> =
iovs.iter()
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Iovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice_mut()?.expect(
"cannot use with shared memories; use `as_unsafe_slice_mut` instead",
))
})
.collect::<Result<_, Error>>()?;

let mut ioslices: Vec<IoSliceMut> = guest_slices
.iter_mut()
Expand All @@ -497,14 +499,16 @@ impl wasi_unstable::WasiUnstable for WasiCtx {
.get_file_mut(u32::from(fd))?
.get_cap_mut(FileCaps::READ | FileCaps::SEEK)?;

let mut guest_slices: Vec<wiggle::GuestSliceMut<u8>> = iovs
.iter()
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Iovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice_mut()?)
})
.collect::<Result<_, Error>>()?;
let mut guest_slices: Vec<wiggle::GuestSliceMut<u8>> =
iovs.iter()
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Iovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice_mut()?.expect(
"cannot use with shared memories; use `as_unsafe_slice_mut` instead",
))
})
.collect::<Result<_, Error>>()?;

let mut ioslices: Vec<IoSliceMut> = guest_slices
.iter_mut()
Expand All @@ -530,7 +534,11 @@ impl wasi_unstable::WasiUnstable for WasiCtx {
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Ciovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice()?)
Ok(iov
.buf
.as_array(iov.buf_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead"))
})
.collect::<Result<_, Error>>()?;

Expand Down Expand Up @@ -559,7 +567,11 @@ impl wasi_unstable::WasiUnstable for WasiCtx {
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Ciovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice()?)
Ok(iov
.buf
.as_array(iov.buf_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead"))
})
.collect::<Result<_, Error>>()?;

Expand Down
88 changes: 58 additions & 30 deletions crates/wasi-common/src/snapshots/preview_1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -521,14 +521,16 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
.get_file_mut(u32::from(fd))?
.get_cap_mut(FileCaps::READ)?;

let mut guest_slices: Vec<wiggle::GuestSliceMut<u8>> = iovs
.iter()
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Iovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice_mut()?)
})
.collect::<Result<_, Error>>()?;
let mut guest_slices: Vec<wiggle::GuestSliceMut<u8>> =
iovs.iter()
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Iovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice_mut()?.expect(
"cannot use with shared memories; use `as_unsafe_slice_mut` instead",
))
})
.collect::<Result<_, Error>>()?;

let mut ioslices: Vec<IoSliceMut> = guest_slices
.iter_mut()
Expand All @@ -550,14 +552,16 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
.get_file_mut(u32::from(fd))?
.get_cap_mut(FileCaps::READ | FileCaps::SEEK)?;

let mut guest_slices: Vec<wiggle::GuestSliceMut<u8>> = iovs
.iter()
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Iovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice_mut()?)
})
.collect::<Result<_, Error>>()?;
let mut guest_slices: Vec<wiggle::GuestSliceMut<u8>> =
iovs.iter()
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Iovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice_mut()?.expect(
"cannot use with shared memories; use `as_unsafe_slice_mut` instead",
))
})
.collect::<Result<_, Error>>()?;

let mut ioslices: Vec<IoSliceMut> = guest_slices
.iter_mut()
Expand All @@ -583,7 +587,11 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Ciovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice()?)
Ok(iov
.buf
.as_array(iov.buf_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead"))
})
.collect::<Result<_, Error>>()?;

Expand Down Expand Up @@ -612,7 +620,11 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Ciovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice()?)
Ok(iov
.buf
.as_array(iov.buf_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead"))
})
.collect::<Result<_, Error>>()?;

Expand Down Expand Up @@ -654,7 +666,10 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
if path_len < path_max_len as usize {
return Err(Error::name_too_long());
}
let mut p_memory = path.as_array(path_len as u32).as_slice_mut()?;
let mut p_memory = path
.as_array(path_len as u32)
.as_slice_mut()?
.expect("cannot use with shared memories; use `as_unsafe_slice_mut` instead");
p_memory.copy_from_slice(path_bytes);
Ok(())
} else {
Expand Down Expand Up @@ -948,7 +963,10 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
if link_len > buf_len as usize {
return Err(Error::range());
}
let mut buf = buf.as_array(link_len as u32).as_slice_mut()?;
let mut buf = buf
.as_array(link_len as u32)
.as_slice_mut()?
.expect("cannot use with shared memories; use `as_unsafe_slice_mut` instead");
buf.copy_from_slice(link_bytes);
Ok(link_len as types::Size)
}
Expand Down Expand Up @@ -1236,7 +1254,10 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
buf: &GuestPtr<'a, u8>,
buf_len: types::Size,
) -> Result<(), Error> {
let mut buf = buf.as_array(buf_len).as_slice_mut()?;
let mut buf = buf
.as_array(buf_len)
.as_slice_mut()?
.expect("cannot use with shared memories; use `as_unsafe_slice_mut` instead");
self.random.try_fill_bytes(buf.deref_mut())?;
Ok(())
}
Expand Down Expand Up @@ -1273,14 +1294,17 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
.get_file_mut(u32::from(fd))?
.get_cap_mut(FileCaps::READ)?;

let mut guest_slices: Vec<wiggle::GuestSliceMut<u8>> = ri_data
.iter()
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Iovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice_mut()?)
})
.collect::<Result<_, Error>>()?;
let mut guest_slices: Vec<wiggle::GuestSliceMut<u8>> =
ri_data
.iter()
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Iovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice_mut()?.expect(
"cannot use with shared memories; use `as_unsafe_slice_mut` instead",
))
})
.collect::<Result<_, Error>>()?;

let mut ioslices: Vec<IoSliceMut> = guest_slices
.iter_mut()
Expand All @@ -1307,7 +1331,11 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
.map(|iov_ptr| {
let iov_ptr = iov_ptr?;
let iov: types::Ciovec = iov_ptr.read()?;
Ok(iov.buf.as_array(iov.buf_len).as_slice()?)
Ok(iov
.buf
.as_array(iov.buf_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead"))
})
.collect::<Result<_, Error>>()?;

Expand Down
30 changes: 24 additions & 6 deletions crates/wasi-crypto/src/wiggle_interfaces/asymmetric_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr
kp_id_ptr: &wiggle::GuestPtr<'_, u8>,
kp_id_max_len: guest_types::Size,
) -> Result<(), guest_types::CryptoErrno> {
let key_id_buf = &mut *kp_id_ptr.as_array(kp_id_max_len).as_slice_mut()?;
let key_id_buf = &mut *kp_id_ptr
.as_array(kp_id_max_len)
.as_slice_mut()?
.expect("cannot use with shared memories; use `as_unsafe_slice_mut` instead");
Ok((&*self).keypair_store_managed(
secrets_manager_handle.into(),
kp_handle.into(),
Expand Down Expand Up @@ -69,7 +72,10 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr
kp_id_len: guest_types::Size,
kp_version: guest_types::Version,
) -> Result<guest_types::Keypair, guest_types::CryptoErrno> {
let kp_id = &*kp_id_ptr.as_array(kp_id_len).as_slice()?;
let kp_id = &*kp_id_ptr
.as_array(kp_id_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead");
Ok((&*self)
.keypair_from_id(secrets_manager_handle.into(), kp_id, Version(kp_version))?
.into())
Expand Down Expand Up @@ -102,7 +108,10 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr
encoding: guest_types::KeypairEncoding,
) -> Result<guest_types::Keypair, guest_types::CryptoErrno> {
let alg_str = &*alg_str.as_str()?;
let encoded = &*encoded_ptr.as_array(encoded_len).as_slice()?;
let encoded = &*encoded_ptr
.as_array(encoded_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead");
Ok((&*self)
.keypair_import(alg_type.into(), alg_str, encoded, encoding.into())?
.into())
Expand All @@ -114,7 +123,10 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr
kp_id_ptr: &wiggle::GuestPtr<'_, u8>,
kp_id_max_len: guest_types::Size,
) -> Result<(guest_types::Size, guest_types::Version), guest_types::CryptoErrno> {
let kp_id_buf = &mut *kp_id_ptr.as_array(kp_id_max_len as _).as_slice_mut()?;
let kp_id_buf = &mut *kp_id_ptr
.as_array(kp_id_max_len as _)
.as_slice_mut()?
.expect("cannot use with shared memories; use `as_unsafe_slice_mut` instead");
let (kp_id, version) = (&*self).keypair_id(kp_handle.into())?;
ensure!(kp_id.len() <= kp_id_buf.len(), CryptoError::Overflow.into());
kp_id_buf.copy_from_slice(&kp_id);
Expand Down Expand Up @@ -156,7 +168,10 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr
encoding: guest_types::PublickeyEncoding,
) -> Result<guest_types::Publickey, guest_types::CryptoErrno> {
let alg_str = &*alg_str.as_str()?;
let encoded = &*encoded_ptr.as_array(encoded_len).as_slice()?;
let encoded = &*encoded_ptr
.as_array(encoded_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead");
Ok((&*self)
.publickey_import(alg_type.into(), alg_str, encoded, encoding.into())?
.into())
Expand Down Expand Up @@ -204,7 +219,10 @@ impl super::wasi_ephemeral_crypto_asymmetric_common::WasiEphemeralCryptoAsymmetr
encoding: guest_types::SecretkeyEncoding,
) -> Result<guest_types::Secretkey, guest_types::CryptoErrno> {
let alg_str = &*alg_str.as_str()?;
let encoded = &*encoded_ptr.as_array(encoded_len).as_slice()?;
let encoded = &*encoded_ptr
.as_array(encoded_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead");
Ok((&*self)
.secretkey_import(alg_type.into(), alg_str, encoded, encoding.into())?
.into())
Expand Down
31 changes: 26 additions & 5 deletions crates/wasi-crypto/src/wiggle_interfaces/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ impl super::wasi_ephemeral_crypto_common::WasiEphemeralCryptoCommon for WasiCryp
value_len: guest_types::Size,
) -> Result<(), guest_types::CryptoErrno> {
let name_str: &str = &*name_str.as_str()?;
let value: &[u8] = { &*value_ptr.as_array(value_len).as_slice()? };
let value: &[u8] = {
&*value_ptr
.as_array(value_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead")
};
Ok((&*self).options_set(options_handle.into(), name_str, value)?)
}

Expand All @@ -40,8 +45,14 @@ impl super::wasi_ephemeral_crypto_common::WasiEphemeralCryptoCommon for WasiCryp
buffer_len: guest_types::Size,
) -> Result<(), guest_types::CryptoErrno> {
let name_str: &str = &*name_str.as_str()?;
let buffer: &'static mut [u8] =
unsafe { std::mem::transmute(&mut *buffer_ptr.as_array(buffer_len).as_slice_mut()?) };
let buffer: &'static mut [u8] = unsafe {
std::mem::transmute(
&mut *buffer_ptr
.as_array(buffer_len)
.as_slice_mut()?
.expect("cannot use with shared memories; use `as_unsafe_slice_mut` instead"),
)
};
Ok((&*self).options_set_guest_buffer(options_handle.into(), name_str, buffer)?)
}

Expand Down Expand Up @@ -72,7 +83,12 @@ impl super::wasi_ephemeral_crypto_common::WasiEphemeralCryptoCommon for WasiCryp
buf_ptr: &wiggle::GuestPtr<'_, u8>,
buf_len: guest_types::Size,
) -> Result<guest_types::Size, guest_types::CryptoErrno> {
let buf: &mut [u8] = { &mut *buf_ptr.as_array(buf_len).as_slice_mut()? };
let buf: &mut [u8] = {
&mut *buf_ptr
.as_array(buf_len)
.as_slice_mut()?
.expect("cannot use with shared memories; use `as_unsafe_slice_mut` instead")
};
Ok((&*self)
.array_output_pull(array_output_handle.into(), buf)?
.try_into()?)
Expand Down Expand Up @@ -107,7 +123,12 @@ impl super::wasi_ephemeral_crypto_common::WasiEphemeralCryptoCommon for WasiCryp
key_id_len: guest_types::Size,
key_version: guest_types::Version,
) -> Result<(), guest_types::CryptoErrno> {
let key_id: &[u8] = { &*key_id_ptr.as_array(key_id_len).as_slice()? };
let key_id: &[u8] = {
&*key_id_ptr
.as_array(key_id_len)
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead")
};
Ok((&*self).secrets_manager_invalidate(
secrets_manager_handle.into(),
key_id,
Expand Down
3 changes: 2 additions & 1 deletion crates/wasi-crypto/src/wiggle_interfaces/key_exchange.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ impl super::wasi_ephemeral_crypto_kx::WasiEphemeralCryptoKx for WasiCryptoCtx {
) -> Result<guest_types::ArrayOutput, guest_types::CryptoErrno> {
let encapsulated_secret = &*encapsulated_secret_ptr
.as_array(encapsulated_secret_len)
.as_slice()?;
.as_slice()?
.expect("cannot use with shared memories; use `to_vec` instead");
Ok((&*self)
.kx_decapsulate(sk_handle.into(), encapsulated_secret)?
.into())
Expand Down
Loading

0 comments on commit 85cc04d

Please sign in to comment.