Skip to content

Commit

Permalink
Don't explicitly check for null function pointers
Browse files Browse the repository at this point in the history 
This commit uses the support from #8162 to skip null function pointer
checks when performing an indirect call. Instead of an explicit check
the segfault from accessing the null function pointer is caught and
annotated with the appropriate trap.

Closes #5291
  • Loading branch information
@alexcrichton
alexcrichton committed Mar 18, 2024
1 parent b651c44 commit 8799f01
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 40 deletions.
35 changes: 20 additions & 15 deletions crates/cranelift/src/func_environ.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1155,11 +1155,6 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> {
self.env
.get_or_init_func_ref_table_elem(self.builder, table_index, callee);

// Check for whether the table element is null, and trap if so.
self.builder
.ins()
.trapz(funcref_ptr, ir::TrapCode::IndirectCallToNull);

// If necessary, check the signature.
match self.env.module.table_plans[table_index].style {
TableStyle::CallerChecksSignature => {
Expand Down Expand Up @@ -1188,10 +1183,13 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> {
.load(sig_id_type, mem_flags, signatures, offset);

// Load the callee ID.
//
// Note that the callee may be null in which case this load may
// trap. If so use the `IndirectCallToNull` trap code.
let mem_flags = ir::MemFlags::trusted().with_readonly();
let callee_sig_id = self.builder.ins().load(
sig_id_type,
mem_flags,
mem_flags.with_trap_code(Some(ir::TrapCode::IndirectCallToNull)),
funcref_ptr,
i32::from(self.env.offsets.ptr.vm_func_ref_type_index()),
);
Expand All @@ -1205,7 +1203,11 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> {
}
}

self.unchecked_call(sig_ref, funcref_ptr, call_args)
// Note that `funcref_ptr` at this point is guaranteed to not be null as
// we've loaded its type which would have otherwise faulted. That means
// that further loads no longer need trap metadata, so use `None` here
// for the next trap code.
self.unchecked_call(sig_ref, funcref_ptr, None, call_args)
}

/// Call a typed function reference.
Expand All @@ -1215,19 +1217,15 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> {
callee: ir::Value,
args: &[ir::Value],
) -> WasmResult<ir::Inst> {
// Check for whether the callee is null, and trap if so.
//
// FIXME: the wasm type system tracks enough information to know whether
// `callee` is a null reference or not. In some situations it can be
// statically known here that `callee` cannot be null in which case this
// null check can be elided. This requires feeding type information from
// can be `None` instead. This requires feeding type information from
// wasmparser's validator into this function, however, which is not
// easily done at this time.
self.builder
.ins()
.trapz(callee, ir::TrapCode::NullReference);
let callee_load_trap_code = Some(ir::TrapCode::NullReference);

self.unchecked_call(sig_ref, callee, args)
self.unchecked_call(sig_ref, callee, callee_load_trap_code, args)
}

/// This calls a function by reference without checking the signature.
Expand All @@ -1239,15 +1237,22 @@ impl<'a, 'func, 'module_env> Call<'a, 'func, 'module_env> {
&mut self,
sig_ref: ir::SigRef,
callee: ir::Value,
callee_load_trap_code: Option<ir::TrapCode>,
call_args: &[ir::Value],
) -> WasmResult<ir::Inst> {
let pointer_type = self.env.pointer_type();

// Dereference callee pointer to get the function address.
//
// Note that this may trap if `callee` hasn't previously been verified
// to be non-null. This means that this load is annotated with an
// optional trap code provided by the caller of `unchecked_call` which
// will handle the case where this is either already known to be
// non-null or may trap.
let mem_flags = ir::MemFlags::trusted().with_readonly();
let func_addr = self.builder.ins().load(
pointer_type,
mem_flags,
mem_flags.with_trap_code(callee_load_trap_code),
callee,
i32::from(self.env.offsets.ptr.vm_func_ref_wasm_call()),
);
Expand Down
24 changes: 20 additions & 4 deletions crates/wasmtime/src/runtime/store.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1595,10 +1595,26 @@ impl StoreOpaque {
/// always zero in these situations which means that the trapping context
/// doesn't have enough information to report the fault address.
pub(crate) fn wasm_fault(&self, pc: usize, addr: usize) -> Option<WasmFault> {
// Explicitly bounds-checked memories with spectre-guards enabled will
// cause out-of-bounds accesses to get routed to address 0, so allow
// wasm instructions to fault on the null address.
if addr == 0 {
// There are a few instances where a "close to zero" pointer is loaded
// and we expect that to happen:
//
// * Explicitly bounds-checked memories with spectre-guards enabled will
// cause out-of-bounds accesses to get routed to address 0, so allow
// wasm instructions to fault on the null address.
// * `call_indirect` when invoking a null function pointer may load data
// from the a `VMFuncRef` whose address is null, meaning any field of
// `VMFuncRef` could be the address of the fault.
//
// In these situations where the address is so small it won't be in any
// instance, so skip the checks below.
if addr <= mem::size_of::<VMFuncRef>() {
const _: () = {
// static-assert that `VMFuncRef` isn't too big to ensure that
// it lives solely within the first page as we currently only
// have the guarantee that the first page of memory is unmapped,
// no more.
assert!(mem::size_of::<VMFuncRef>() <= 512);
};
return None;
}

Expand Down
3 changes: 1 addition & 2 deletions tests/disas/icall-simd.wat
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,10 @@
;; @0033 jump block3(v20)
;;
;; block3(v15: i64):
;; @0033 trapz v15, icall_null
;; @0033 v21 = global_value.i64 gv3
;; @0033 v22 = load.i64 notrap aligned readonly v21+64
;; @0033 v23 = load.i32 notrap aligned readonly v22
;; @0033 v24 = load.i32 notrap aligned readonly v15+24
;; @0033 v24 = load.i32 icall_null aligned readonly v15+24
;; @0033 v25 = icmp eq v24, v23
;; @0033 trapz v25, bad_sig
;; @0033 v26 = load.i64 notrap aligned readonly v15+16
Expand Down
3 changes: 1 addition & 2 deletions tests/disas/icall.wat
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,10 @@
;; @0033 jump block3(v20)
;;
;; block3(v15: i64):
;; @0033 trapz v15, icall_null
;; @0033 v21 = global_value.i64 gv3
;; @0033 v22 = load.i64 notrap aligned readonly v21+64
;; @0033 v23 = load.i32 notrap aligned readonly v22
;; @0033 v24 = load.i32 notrap aligned readonly v15+24
;; @0033 v24 = load.i32 icall_null aligned readonly v15+24
;; @0033 v25 = icmp eq v24, v23
;; @0033 trapz v25, bad_sig
;; @0033 v26 = load.i64 notrap aligned readonly v15+16
Expand Down
22 changes: 5 additions & 17 deletions tests/disas/typed-funcrefs.wat
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,22 +113,16 @@
;; @0036 jump block3(v24)
;;
;; block3(v19: i64):
;; @0038 brif v19, block9, block8
;;
;; block8 cold:
;; @0038 trap null_reference
;;
;; block9:
;; @0038 v25 = load.i64 notrap aligned readonly v19+16
;; @0038 v25 = load.i64 null_reference aligned readonly v19+16
;; @0038 v26 = load.i64 notrap aligned readonly v19+32
;; @0038 v27 = call_indirect sig1, v25(v26, v0, v2, v3, v4, v5)
;; v80 = iconst.i8 0
;; @0049 brif v80, block10, block11 ; v80 = 0
;; @0049 brif v80, block8, block9 ; v80 = 0
;;
;; block10 cold:
;; block8 cold:
;; @0049 trap table_oob
;;
;; block11:
;; block9:
;; @0049 v38 = load.i64 notrap aligned v0+72
;; v81 = iconst.i8 0
;; v78 = iconst.i64 16
Expand All @@ -148,13 +142,7 @@
;; @0049 jump block5(v50)
;;
;; block5(v45: i64):
;; @004b brif v45, block13, block12
;;
;; block12 cold:
;; @004b trap null_reference
;;
;; block13:
;; @004b v51 = load.i64 notrap aligned readonly v45+16
;; @004b v51 = load.i64 null_reference aligned readonly v45+16
;; @004b v52 = load.i64 notrap aligned readonly v45+32
;; @004b v53 = call_indirect sig1, v51(v52, v0, v2, v3, v4, v5)
;; @0054 jump block1
Expand Down

0 comments on commit 8799f01

Please sign in to comment.