From 18f8a58d725863dc3b1d625eb2e6fe00c899ba29 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Thu, 3 Sep 2020 15:23:11 -0700 Subject: [PATCH 1/4] Fix soundness issue in vm::Global The `unsafe impl` for `Send` and `Sync` relies on the global being locked when it's being accessed; the get and get_mut functions did not (and could not) do this, so we replace them with `get` and `set` methods which operate on `Value`s directly. Additionally the `get_mut` function took a `&self` and returned a `&mut` to the underlying data which allows for aliased mutable references to the same data which is another soundness issue. --- lib/api/src/externals/global.rs | 35 +++++--------------- lib/vm/src/global.rs | 58 +++++++++++++++++++++++++++++---- 2 files changed, 59 insertions(+), 34 deletions(-) diff --git a/lib/api/src/externals/global.rs b/lib/api/src/externals/global.rs index 7203700db76..c1907e3afac 100644 --- a/lib/api/src/externals/global.rs +++ b/lib/api/src/externals/global.rs @@ -1,7 +1,7 @@ use crate::exports::{ExportError, Exportable}; use crate::externals::Extern; use crate::store::{Store, StoreObject}; -use crate::types::{Val, ValType}; +use crate::types::Val; use crate::GlobalType; use crate::Mutability; use crate::RuntimeError; @@ -42,14 +42,9 @@ impl Global { ty: val.ty(), }); unsafe { - match val { - Val::I32(x) => *global.get_mut().as_i32_mut() = x, - Val::I64(x) => *global.get_mut().as_i64_mut() = x, - Val::F32(x) => *global.get_mut().as_f32_mut() = x, - Val::F64(x) => *global.get_mut().as_f64_mut() = x, - Val::V128(x) => *global.get_mut().as_u128_bits_mut() = x.to_ne_bytes(), - _ => return Err(RuntimeError::new(format!("create_global for {:?}", val))), - } + global + .set(val.clone()) + .map_err(|e| RuntimeError::new(format!("create global for {:?}: {}", val, e)))?; }; Ok(Global { @@ -70,16 +65,7 @@ impl Global { /// Retrieves the current value [`Val`] that the Global has. pub fn get(&self) -> Val { - unsafe { - let definition = self.global.get(); - match self.ty().ty { - ValType::I32 => Val::from(*definition.as_i32()), - ValType::I64 => Val::from(*definition.as_i64()), - ValType::F32 => Val::F32(*definition.as_f32()), - ValType::F64 => Val::F64(*definition.as_f64()), - _ => unimplemented!("Global::get for {:?}", self.ty().ty), - } - } + self.global.get() } /// Sets a custom value [`Val`] to the runtime Global. @@ -106,14 +92,9 @@ impl Global { return Err(RuntimeError::new("cross-`Store` values are not supported")); } unsafe { - let definition = self.global.get_mut(); - match val { - Val::I32(i) => *definition.as_i32_mut() = i, - Val::I64(i) => *definition.as_i64_mut() = i, - Val::F32(f) => *definition.as_f32_mut() = f, - Val::F64(f) => *definition.as_f64_mut() = f, - _ => unimplemented!("Global::set for {:?}", val.ty()), - } + self.global + .set(val) + .map_err(|e| RuntimeError::new(format!("Failed to set global: {}", e)))?; } Ok(()) } diff --git a/lib/vm/src/global.rs b/lib/vm/src/global.rs index 1f479072a7b..f33ccaf99a0 100644 --- a/lib/vm/src/global.rs +++ b/lib/vm/src/global.rs @@ -3,7 +3,7 @@ use std::cell::UnsafeCell; use std::ptr::NonNull; use std::sync::Mutex; use thiserror::Error; -use wasmer_types::{GlobalType, Type}; +use wasmer_types::{GlobalType, Mutability, Type, Value}; #[derive(Debug)] /// TODO: figure out a decent name for this thing @@ -63,13 +63,57 @@ impl Global { unsafe { NonNull::new_unchecked(ptr) } } - /// Get a reference to the definition - pub fn get(&self) -> &VMGlobalDefinition { - unsafe { &*self.vm_global_definition.get() } + /// Get a value from the global. + pub fn get(&self) -> Value { + let _global_guard = self.lock.lock().unwrap(); + unsafe { + let definition = &*self.vm_global_definition.get(); + match self.ty().ty { + Type::I32 => Value::from(*definition.as_i32()), + Type::I64 => Value::from(*definition.as_i64()), + Type::F32 => Value::F32(*definition.as_f32()), + Type::F64 => Value::F64(*definition.as_f64()), + Type::V128 => Value::V128(*definition.as_u128()), + _ => unimplemented!("Global::get for {:?}", self.ty), + } + } + } + + /// Set a value for the global. + /// + /// # Safety + /// The caller should check that the `val` comes from the same store as this global. + pub unsafe fn set(&self, val: Value) -> Result<(), GlobalError> { + let _global_guard = self.lock.lock().unwrap(); + if self.ty().mutability != Mutability::Var { + return Err(GlobalError::ImmutableGlobalCannotBeSet); + } + if val.ty() != self.ty().ty { + return Err(GlobalError::IncorrectType { + expected: self.ty.ty, + found: val.ty(), + }); + } + self.set_unchecked(val) } - /// Get a mutable reference to the definition - pub fn get_mut(&self) -> &mut VMGlobalDefinition { - unsafe { &mut *self.vm_global_definition.get() } + /// Set a value from the global (unchecked) + /// + /// # Safety + /// The caller should check that the `val` comes from the same store as this global. + /// The caller should also ensure that this global is synchronized. Otherwise, use + /// `set` instead. + pub unsafe fn set_unchecked(&self, val: Value) -> Result<(), GlobalError> { + // ideally we'd use atomics here + let definition = &mut *self.vm_global_definition.get(); + match val { + Value::I32(i) => *definition.as_i32_mut() = i, + Value::I64(i) => *definition.as_i64_mut() = i, + Value::F32(f) => *definition.as_f32_mut() = f, + Value::F64(f) => *definition.as_f64_mut() = f, + Value::V128(x) => *definition.as_u128_bits_mut() = x.to_ne_bytes(), + _ => unimplemented!("Global::set for {:?}", val.ty()), + } + Ok(()) } } From db1ee4c1f9c0bccd8759b3ddb7177c28e938e55f Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Thu, 3 Sep 2020 15:26:25 -0700 Subject: [PATCH 2/4] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2480b2996a2..c4362669355 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## **[Unreleased]** +- [#1590](https://github.com/wasmerio/wasmer/pull/1590) Fix soundness issue in API on vm::Global - [#1566](https://github.com/wasmerio/wasmer/pull/1566) Add support for opening special Unix files to the WASI FS ## TODO: 1.0.0-alpha1.0 From b40714407badbd1576f039397e882af5d637095c Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Thu, 3 Sep 2020 16:30:04 -0700 Subject: [PATCH 3/4] Add updates from feedback --- CHANGELOG.md | 2 +- lib/vm/src/global.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4362669355..9d24118bfb4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ # Changelog ## **[Unreleased]** -- [#1590](https://github.com/wasmerio/wasmer/pull/1590) Fix soundness issue in API on vm::Global +- [#1590](https://github.com/wasmerio/wasmer/pull/1590) Fix soundness issue in API of vm::Global - [#1566](https://github.com/wasmerio/wasmer/pull/1566) Add support for opening special Unix files to the WASI FS ## TODO: 1.0.0-alpha1.0 diff --git a/lib/vm/src/global.rs b/lib/vm/src/global.rs index f33ccaf99a0..d1ff6937fd4 100644 --- a/lib/vm/src/global.rs +++ b/lib/vm/src/global.rs @@ -69,8 +69,8 @@ impl Global { unsafe { let definition = &*self.vm_global_definition.get(); match self.ty().ty { - Type::I32 => Value::from(*definition.as_i32()), - Type::I64 => Value::from(*definition.as_i64()), + Type::I32 => Value::I32(*definition.as_i32()), + Type::I64 => Value::I64(*definition.as_i64()), Type::F32 => Value::F32(*definition.as_f32()), Type::F64 => Value::F64(*definition.as_f64()), Type::V128 => Value::V128(*definition.as_u128()), @@ -104,7 +104,7 @@ impl Global { /// The caller should also ensure that this global is synchronized. Otherwise, use /// `set` instead. pub unsafe fn set_unchecked(&self, val: Value) -> Result<(), GlobalError> { - // ideally we'd use atomics here + // ideally we'd use atomics for the global value rather than needing to lock it let definition = &mut *self.vm_global_definition.get(); match val { Value::I32(i) => *definition.as_i32_mut() = i, From c2a10c404612fd0ef5c968d679f1f48f19e9820d Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Thu, 3 Sep 2020 16:53:57 -0700 Subject: [PATCH 4/4] Remove redundant checks form api::Global::set --- lib/api/src/externals/global.rs | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/lib/api/src/externals/global.rs b/lib/api/src/externals/global.rs index c1907e3afac..b922a854c08 100644 --- a/lib/api/src/externals/global.rs +++ b/lib/api/src/externals/global.rs @@ -43,7 +43,7 @@ impl Global { }); unsafe { global - .set(val.clone()) + .set_unchecked(val.clone()) .map_err(|e| RuntimeError::new(format!("create global for {:?}: {}", val, e)))?; }; @@ -76,25 +76,13 @@ impl Global { /// * The global is not mutable /// * The type of the `Val` doesn't matches the Global type. pub fn set(&self, val: Val) -> Result<(), RuntimeError> { - if self.ty().mutability != Mutability::Var { - return Err(RuntimeError::new( - "immutable global cannot be set".to_string(), - )); - } - if val.ty() != self.ty().ty { - return Err(RuntimeError::new(format!( - "global of type {:?} cannot be set to {:?}", - self.ty().ty, - val.ty() - ))); - } if !val.comes_from_same_store(&self.store) { return Err(RuntimeError::new("cross-`Store` values are not supported")); } unsafe { self.global .set(val) - .map_err(|e| RuntimeError::new(format!("Failed to set global: {}", e)))?; + .map_err(|e| RuntimeError::new(format!("{}", e)))?; } Ok(()) }