From df2b448993500a4faf368c2ac1ff2232ea272dd7 Mon Sep 17 00:00:00 2001 From: Brian Anderson Date: Mon, 6 Dec 2021 11:26:46 -0600 Subject: [PATCH] Fix incorrect nonoverlapping test in sol_memcpy (#21007) Thanks! --- programs/bpf_loader/src/syscalls.rs | 50 +++++++++++++++++++++-------- sdk/program/src/program_stubs.rs | 34 +++++++++++++++++++- sdk/src/feature_set.rs | 5 +++ 3 files changed, 74 insertions(+), 15 deletions(-) diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index a9aa33f3c5..3c813c2f26 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -25,9 +25,10 @@ use { epoch_schedule::EpochSchedule, feature_set::{ blake3_syscall_enabled, demote_program_write_locks, disable_fees_sysvar, - do_support_realloc, libsecp256k1_0_5_upgrade_enabled, - prevent_calling_precompiles_as_programs, return_data_syscall_enabled, - secp256k1_recover_syscall_enabled, sol_log_data_syscall_enabled, + do_support_realloc, fixed_memcpy_nonoverlapping_check, + libsecp256k1_0_5_upgrade_enabled, prevent_calling_precompiles_as_programs, + return_data_syscall_enabled, secp256k1_recover_syscall_enabled, + sol_log_data_syscall_enabled, }, hash::{Hasher, HASH_BYTES}, instruction::{AccountMeta, Instruction, InstructionError}, @@ -36,6 +37,7 @@ use { native_loader, precompiles::is_precompile, program::MAX_RETURN_DATA, + program_stubs::is_nonoverlapping, pubkey::{Pubkey, PubkeyError, MAX_SEEDS, MAX_SEED_LEN}, rent::Rent, secp256k1_recover::{ @@ -1260,7 +1262,9 @@ impl<'a, 'b> SyscallObject for SyscallKeccak256<'a, 'b> { } } -fn check_overlapping(src_addr: u64, dst_addr: u64, n: u64) -> bool { +/// This function is incorrect due to arithmetic overflow and only exists for +/// backwards compatibility. Instead use program_stubs::is_nonoverlapping. +fn check_overlapping_do_not_use(src_addr: u64, dst_addr: u64, n: u64) -> bool { (src_addr <= dst_addr && src_addr + n > dst_addr) || (dst_addr <= src_addr && dst_addr + n > src_addr) } @@ -1280,9 +1284,27 @@ impl<'a, 'b> SyscallObject for SyscallMemcpy<'a, 'b> { memory_mapping: &MemoryMapping, result: &mut Result>, ) { - if check_overlapping(src_addr, dst_addr, n) { - *result = Err(SyscallError::CopyOverlapping.into()); - return; + let invoke_context = question_mark!( + self.invoke_context + .try_borrow() + .map_err(|_| SyscallError::InvokeContextBorrowFailed), + result + ); + let use_fixed_nonoverlapping_check = invoke_context + .feature_set + .is_active(&fixed_memcpy_nonoverlapping_check::id()); + + #[allow(clippy::collapsible_else_if)] + if use_fixed_nonoverlapping_check { + if !is_nonoverlapping(src_addr, dst_addr, n) { + *result = Err(SyscallError::CopyOverlapping.into()); + return; + } + } else { + if check_overlapping_do_not_use(src_addr, dst_addr, n) { + *result = Err(SyscallError::CopyOverlapping.into()); + return; + } } let invoke_context = question_mark!( @@ -3718,13 +3740,13 @@ mod tests { #[test] fn test_overlapping() { - assert!(!check_overlapping(10, 7, 3)); - assert!(check_overlapping(10, 8, 3)); - assert!(check_overlapping(10, 9, 3)); - assert!(check_overlapping(10, 10, 3)); - assert!(check_overlapping(10, 11, 3)); - assert!(check_overlapping(10, 12, 3)); - assert!(!check_overlapping(10, 13, 3)); + assert!(!check_overlapping_do_not_use(10, 7, 3)); + assert!(check_overlapping_do_not_use(10, 8, 3)); + assert!(check_overlapping_do_not_use(10, 9, 3)); + assert!(check_overlapping_do_not_use(10, 10, 3)); + assert!(check_overlapping_do_not_use(10, 11, 3)); + assert!(check_overlapping_do_not_use(10, 12, 3)); + assert!(!check_overlapping_do_not_use(10, 13, 3)); } fn call_program_address_common( diff --git a/sdk/program/src/program_stubs.rs b/sdk/program/src/program_stubs.rs index b3542062ad..b9f6257514 100644 --- a/sdk/program/src/program_stubs.rs +++ b/sdk/program/src/program_stubs.rs @@ -54,7 +54,7 @@ pub trait SyscallStubs: Sync + Send { unsafe fn sol_memcpy(&self, dst: *mut u8, src: *const u8, n: usize) { // cannot be overlapping assert!( - !(dst as usize + n > src as usize && src as usize > dst as usize), + is_nonoverlapping(src as usize, dst as usize, n), "memcpy does not support overlapping regions" ); std::ptr::copy_nonoverlapping(src, dst, n as usize); @@ -176,3 +176,35 @@ pub(crate) fn sol_set_return_data(data: &[u8]) { pub(crate) fn sol_log_data(data: &[&[u8]]) { SYSCALL_STUBS.read().unwrap().sol_log_data(data) } + +/// Check that two regions do not overlap. +/// +/// Adapted from libcore, hidden to share with bpf_loader without being part of +/// the API surface. +#[doc(hidden)] +pub fn is_nonoverlapping(src: N, dst: N, count: N) -> bool +where + N: Ord + std::ops::Sub, + ::Output: Ord, +{ + let diff = if src > dst { src - dst } else { dst - src }; + // If the absolute distance between the ptrs is at least as big as the size of the buffer, + // they do not overlap. + diff >= count +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_is_nonoverlapping() { + assert!(is_nonoverlapping(10, 7, 3)); + assert!(!is_nonoverlapping(10, 8, 3)); + assert!(!is_nonoverlapping(10, 9, 3)); + assert!(!is_nonoverlapping(10, 10, 3)); + assert!(!is_nonoverlapping(10, 11, 3)); + assert!(!is_nonoverlapping(10, 12, 3)); + assert!(is_nonoverlapping(10, 13, 3)); + } +} diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index cef24b5b6f..b773394147 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -263,6 +263,10 @@ pub mod reject_empty_instruction_without_program { solana_sdk::declare_id!("9kdtFSrXHQg3hKkbXkQ6trJ3Ja1xpJ22CTFSNAciEwmL"); } +pub mod fixed_memcpy_nonoverlapping_check { + solana_sdk::declare_id!("36PRUK2Dz6HWYdG9SpjeAsF5F3KxnFCakA2BZMbtMhSb"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -323,6 +327,7 @@ lazy_static! { (spl_token_v3_3_0_release::id(), "spl-token v3.3.0 release"), (leave_nonce_on_success::id(), "leave nonce as is on success"), (reject_empty_instruction_without_program::id(), "fail instructions which have native_loader as program_id directly"), + (fixed_memcpy_nonoverlapping_check::id(), "use correct check for nonoverlapping regions in memcpy syscall"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()