From 4da47a418061fb429869ff1a2ad284f2e674856d Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Thu, 28 Dec 2023 15:06:07 -0500 Subject: [PATCH] Fix integer overflow ICEs from round_up_to_next_multiple_of --- src/tools/miri/src/helpers.rs | 8 -------- src/tools/miri/src/lib.rs | 1 + src/tools/miri/src/shims/intrinsics/simd.rs | 9 +++------ src/tools/miri/src/shims/unix/mem.rs | 22 ++++++++++++++++++--- src/tools/miri/tests/pass-dep/shims/mmap.rs | 21 ++++++++++++++++++++ 5 files changed, 44 insertions(+), 17 deletions(-) diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index 98f646da6b69..14ba69b898b1 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/src/helpers.rs @@ -1233,11 +1233,3 @@ pub(crate) fn simd_element_to_bool(elem: ImmTy<'_, Provenance>) -> InterpResult< _ => throw_ub_format!("each element of a SIMD mask must be all-0-bits or all-1-bits"), }) } - -// This looks like something that would be nice to have in the standard library... -pub(crate) fn round_to_next_multiple_of(x: u64, divisor: u64) -> u64 { - assert_ne!(divisor, 0); - // divisor is nonzero; multiplication cannot overflow since we just divided - #[allow(clippy::arithmetic_side_effects)] - return (x.checked_add(divisor - 1).unwrap() / divisor) * divisor; -} diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index 4e9febf02050..cacd02bbfaee 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -11,6 +11,7 @@ #![feature(round_ties_even)] #![feature(let_chains)] #![feature(lint_reasons)] +#![feature(int_roundings)] // Configure clippy and other lints #![allow( clippy::collapsible_else_if, diff --git a/src/tools/miri/src/shims/intrinsics/simd.rs b/src/tools/miri/src/shims/intrinsics/simd.rs index d749182ed5e7..ea2d104694af 100644 --- a/src/tools/miri/src/shims/intrinsics/simd.rs +++ b/src/tools/miri/src/shims/intrinsics/simd.rs @@ -4,10 +4,7 @@ use rustc_middle::{mir, ty, ty::FloatTy}; use rustc_span::{sym, Symbol}; use rustc_target::abi::{Endian, HasDataLayout}; -use crate::helpers::{ - bool_to_simd_element, check_arg_count, round_to_next_multiple_of, simd_element_to_bool, ToHost, - ToSoft, -}; +use crate::helpers::{bool_to_simd_element, check_arg_count, simd_element_to_bool, ToHost, ToSoft}; use crate::*; #[derive(Copy, Clone)] @@ -407,7 +404,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let (yes, yes_len) = this.operand_to_simd(yes)?; let (no, no_len) = this.operand_to_simd(no)?; let (dest, dest_len) = this.place_to_simd(dest)?; - let bitmask_len = round_to_next_multiple_of(dest_len, 8); + let bitmask_len = dest_len.next_multiple_of(8); // The mask must be an integer or an array. assert!( @@ -453,7 +450,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { "bitmask" => { let [op] = check_arg_count(args)?; let (op, op_len) = this.operand_to_simd(op)?; - let bitmask_len = round_to_next_multiple_of(op_len, 8); + let bitmask_len = op_len.next_multiple_of(8); // Returns either an unsigned integer or array of `u8`. assert!( diff --git a/src/tools/miri/src/shims/unix/mem.rs b/src/tools/miri/src/shims/unix/mem.rs index 5aa514715bc3..d7dc17fa89f1 100644 --- a/src/tools/miri/src/shims/unix/mem.rs +++ b/src/tools/miri/src/shims/unix/mem.rs @@ -14,7 +14,7 @@ //! munmap shim which would partily unmap a region of address space previously mapped by mmap will //! report UB. -use crate::{helpers::round_to_next_multiple_of, *}; +use crate::*; use rustc_target::abi::Size; impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} @@ -96,7 +96,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } let align = this.machine.page_align(); - let map_length = round_to_next_multiple_of(length, this.machine.page_size); + let Some(map_length) = length.checked_next_multiple_of(this.machine.page_size) else { + this.set_last_error(Scalar::from_i32(this.eval_libc_i32("EINVAL")))?; + return Ok(this.eval_libc("MAP_FAILED")); + }; + if map_length > this.target_usize_max() { + this.set_last_error(Scalar::from_i32(this.eval_libc_i32("EINVAL")))?; + return Ok(this.eval_libc("MAP_FAILED")); + } let ptr = this.allocate_ptr(Size::from_bytes(map_length), align, MiriMemoryKind::Mmap.into())?; @@ -129,7 +136,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { return Ok(Scalar::from_i32(-1)); } - let length = Size::from_bytes(round_to_next_multiple_of(length, this.machine.page_size)); + let Some(length) = length.checked_next_multiple_of(this.machine.page_size) else { + this.set_last_error(Scalar::from_i32(this.eval_libc_i32("EINVAL")))?; + return Ok(Scalar::from_i32(-1)); + }; + if length > this.target_usize_max() { + this.set_last_error(Scalar::from_i32(this.eval_libc_i32("EINVAL")))?; + return Ok(this.eval_libc("MAP_FAILED")); + } + + let length = Size::from_bytes(length); this.deallocate_ptr( addr, Some((length, this.machine.page_align())), diff --git a/src/tools/miri/tests/pass-dep/shims/mmap.rs b/src/tools/miri/tests/pass-dep/shims/mmap.rs index 518f2ea3e6f1..e19f54d0687d 100644 --- a/src/tools/miri/tests/pass-dep/shims/mmap.rs +++ b/src/tools/miri/tests/pass-dep/shims/mmap.rs @@ -90,9 +90,30 @@ fn test_mmap() { assert_eq!(Error::last_os_error().raw_os_error().unwrap(), libc::ENOTSUP); } + // We report an error for mappings whose length cannot be rounded up to a multiple of + // the page size. + let ptr = unsafe { + libc::mmap( + ptr::null_mut(), + usize::MAX - 1, + libc::PROT_READ | libc::PROT_WRITE, + libc::MAP_PRIVATE | libc::MAP_ANONYMOUS, + -1, + 0, + ) + }; + assert_eq!(ptr, libc::MAP_FAILED); + + // We report an error when trying to munmap an address which is not a multiple of the page size let res = unsafe { libc::munmap(ptr::invalid_mut(1), page_size) }; assert_eq!(res, -1); assert_eq!(Error::last_os_error().raw_os_error().unwrap(), libc::EINVAL); + + // We report an error when trying to munmap a length that cannot be rounded up to a multiple of + // the page size. + let res = unsafe { libc::munmap(ptr::invalid_mut(page_size), usize::MAX - 1) }; + assert_eq!(res, -1); + assert_eq!(Error::last_os_error().raw_os_error().unwrap(), libc::EINVAL); } #[cfg(target_os = "linux")]