diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index 112b8a890..18cbf3648 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -26,7 +26,7 @@ declare_syscall!( .feature_set .is_active(&check_physical_overlapping::id()); - if !is_nonoverlapping(src_addr, dst_addr, n) { + if !is_nonoverlapping(src_addr, n, dst_addr, n) { return Err(SyscallError::CopyOverlapping.into()); } @@ -47,7 +47,7 @@ declare_syscall!( )? .as_ptr(); if do_check_physical_overlapping - && !is_nonoverlapping(src_ptr as usize, dst_ptr as usize, n as usize) + && !is_nonoverlapping(src_ptr as usize, n as usize, dst_ptr as usize, n as usize) { unsafe { std::ptr::copy(src_ptr, dst_ptr, n as usize); diff --git a/programs/bpf_loader/src/syscalls/mod.rs b/programs/bpf_loader/src/syscalls/mod.rs index e5d9be804..fe44cae71 100644 --- a/programs/bpf_loader/src/syscalls/mod.rs +++ b/programs/bpf_loader/src/syscalls/mod.rs @@ -30,7 +30,8 @@ use { entrypoint::{BPF_ALIGN_OF_U128, MAX_PERMITTED_DATA_INCREASE, SUCCESS}, feature_set::FeatureSet, feature_set::{ - self, blake3_syscall_enabled, check_physical_overlapping, curve25519_syscall_enabled, + self, blake3_syscall_enabled, check_physical_overlapping, + check_syscall_outputs_do_not_overlap, curve25519_syscall_enabled, disable_cpi_setting_executable_and_rent_epoch, disable_fees_sysvar, enable_early_verification_of_account_modifications, libsecp256k1_0_5_upgrade_enabled, limit_secp256k1_recovery_id, stop_sibling_instruction_search_at_parent, @@ -649,10 +650,21 @@ declare_syscall!( let address = translate_slice_mut::( memory_mapping, address_addr, - 32, + std::mem::size_of::() as u64, invoke_context.get_check_aligned(), invoke_context.get_check_size(), )?; + if !is_nonoverlapping( + bump_seed_ref as *const _ as usize, + std::mem::size_of_val(bump_seed_ref), + address.as_ptr() as usize, + std::mem::size_of::(), + ) && invoke_context + .feature_set + .is_active(&check_syscall_outputs_do_not_overlap::id()) + { + return Err(SyscallError::CopyOverlapping.into()); + } *bump_seed_ref = bump_seed[0]; address.copy_from_slice(new_address.as_ref()); return Ok(0); @@ -1432,6 +1444,18 @@ declare_syscall!( invoke_context.get_check_aligned(), )?; + if !is_nonoverlapping( + to_slice.as_ptr() as usize, + length as usize, + program_id_result as *const _ as usize, + std::mem::size_of::(), + ) && invoke_context + .feature_set + .is_active(&check_syscall_outputs_do_not_overlap::id()) + { + return Err(SyscallError::CopyOverlapping.into()); + } + *program_id_result = *program_id; } @@ -1490,17 +1514,14 @@ declare_syscall!( } if let Some(instruction_context) = found_instruction_context { - let ProcessedSiblingInstruction { - data_len, - accounts_len, - } = translate_type_mut::( + let result_header = translate_type_mut::( memory_mapping, meta_addr, invoke_context.get_check_aligned(), )?; - if *data_len == (instruction_context.get_instruction_data().len() as u64) - && *accounts_len + if result_header.data_len == (instruction_context.get_instruction_data().len() as u64) + && result_header.accounts_len == (instruction_context.get_number_of_instruction_accounts() as u64) { let program_id = translate_type_mut::( @@ -1511,18 +1532,58 @@ declare_syscall!( let data = translate_slice_mut::( memory_mapping, data_addr, - *data_len as u64, + result_header.data_len as u64, invoke_context.get_check_aligned(), invoke_context.get_check_size(), )?; let accounts = translate_slice_mut::( memory_mapping, accounts_addr, - *accounts_len as u64, + result_header.accounts_len as u64, invoke_context.get_check_aligned(), invoke_context.get_check_size(), )?; + if (!is_nonoverlapping( + result_header as *const _ as usize, + std::mem::size_of::(), + program_id as *const _ as usize, + std::mem::size_of::(), + ) || !is_nonoverlapping( + result_header as *const _ as usize, + std::mem::size_of::(), + accounts.as_ptr() as usize, + std::mem::size_of::() + .saturating_mul(result_header.accounts_len as usize), + ) || !is_nonoverlapping( + result_header as *const _ as usize, + std::mem::size_of::(), + data.as_ptr() as usize, + result_header.data_len as usize, + ) || !is_nonoverlapping( + program_id as *const _ as usize, + std::mem::size_of::(), + data.as_ptr() as usize, + result_header.data_len as usize, + ) || !is_nonoverlapping( + program_id as *const _ as usize, + std::mem::size_of::(), + accounts.as_ptr() as usize, + std::mem::size_of::() + .saturating_mul(result_header.accounts_len as usize), + ) || !is_nonoverlapping( + data.as_ptr() as usize, + result_header.data_len as usize, + accounts.as_ptr() as usize, + std::mem::size_of::() + .saturating_mul(result_header.accounts_len as usize), + )) && invoke_context + .feature_set + .is_active(&check_syscall_outputs_do_not_overlap::id()) + { + return Err(SyscallError::CopyOverlapping.into()); + } + *program_id = *instruction_context .get_last_program_key(invoke_context.transaction_context) .map_err(SyscallError::InstructionError)?; @@ -1548,8 +1609,9 @@ declare_syscall!( .map_err(SyscallError::InstructionError)?; accounts.clone_from_slice(account_metas.as_slice()); } - *data_len = instruction_context.get_instruction_data().len() as u64; - *accounts_len = instruction_context.get_number_of_instruction_accounts() as u64; + result_header.data_len = instruction_context.get_instruction_data().len() as u64; + result_header.accounts_len = + instruction_context.get_number_of_instruction_accounts() as u64; return Ok(true as u64); } Ok(false as u64) @@ -1603,7 +1665,7 @@ declare_syscall!( let instruction_context = transaction_context .get_current_instruction_context() .map_err(SyscallError::InstructionError)?; - let updates = translate_slice_mut::( + let updates = translate_slice::( memory_mapping, updates_addr, updates_count, @@ -1621,7 +1683,7 @@ declare_syscall!( unsafe { std::mem::transmute::<_, TransactionContextAttribute>(update.attribute) }; match attribute { TransactionContextAttribute::TransactionAccountOwner => { - let owner_pubkey = translate_type_mut::( + let owner_pubkey = translate_type::( memory_mapping, update.value, invoke_context.get_check_aligned(), @@ -3620,6 +3682,7 @@ mod tests { invoke_context: &'a mut InvokeContext<'b>, seeds: &[&[u8]], program_id: &Pubkey, + overlap_outputs: bool, syscall: SyscallFunction<&'a mut InvokeContext<'b>>, ) -> Result<(Pubkey, u8), EbpfError> { const SEEDS_VA: u64 = 0x100000000; @@ -3687,7 +3750,11 @@ mod tests { seeds.len() as u64, PROGRAM_ID_VA, ADDRESS_VA, - BUMP_SEED_VA, + if overlap_outputs { + ADDRESS_VA + } else { + BUMP_SEED_VA + }, &mut memory_mapping, &mut result, ); @@ -3703,6 +3770,7 @@ mod tests { invoke_context, seeds, address, + false, SyscallCreateProgramAddress::call, )?; Ok(address) @@ -3717,10 +3785,85 @@ mod tests { invoke_context, seeds, address, + false, SyscallTryFindProgramAddress::call, ) } + #[test] + fn test_set_and_get_return_data() { + const SRC_VA: u64 = 0x100000000; + const DST_VA: u64 = 0x200000000; + const PROGRAM_ID_VA: u64 = 0x300000000; + let data = vec![42; 24]; + let mut data_buffer = vec![0; 16]; + let mut id_buffer = vec![0; 32]; + + let config = Config::default(); + let mut memory_mapping = MemoryMapping::new( + vec![ + MemoryRegion::new_readonly(&data, SRC_VA), + MemoryRegion::new_writable(&mut data_buffer, DST_VA), + MemoryRegion::new_writable(&mut id_buffer, PROGRAM_ID_VA), + ], + &config, + ) + .unwrap(); + + prepare_mockup!( + invoke_context, + transaction_context, + program_id, + bpf_loader::id(), + ); + + let mut result = ProgramResult::Ok(0); + SyscallSetReturnData::call( + &mut invoke_context, + SRC_VA, + data.len() as u64, + 0, + 0, + 0, + &mut memory_mapping, + &mut result, + ); + assert_eq!(result.unwrap(), 0); + + let mut result = ProgramResult::Ok(0); + SyscallGetReturnData::call( + &mut invoke_context, + DST_VA, + data_buffer.len() as u64, + PROGRAM_ID_VA, + 0, + 0, + &mut memory_mapping, + &mut result, + ); + assert_eq!(result.unwrap() as usize, data.len()); + assert_eq!(data.get(0..data_buffer.len()).unwrap(), data_buffer); + assert_eq!(id_buffer, program_id.to_bytes()); + + let mut result = ProgramResult::Ok(0); + SyscallGetReturnData::call( + &mut invoke_context, + PROGRAM_ID_VA, + data_buffer.len() as u64, + PROGRAM_ID_VA, + 0, + 0, + &mut memory_mapping, + &mut result, + ); + assert!(matches!( + result, + ProgramResult::Err(EbpfError::UserError(error)) if error.downcast_ref::().unwrap() == &BpfError::SyscallError( + SyscallError::CopyOverlapping + ), + )); + } + #[test] fn test_syscall_sol_get_processed_sibling_instruction() { let transaction_accounts = (0..9) @@ -3859,6 +4002,28 @@ mod tests { &mut result, ); assert_eq!(result.unwrap(), 0); + + invoke_context + .get_compute_meter() + .borrow_mut() + .mock_set_remaining(syscall_base_cost); + let mut result = ProgramResult::Ok(0); + SyscallGetProcessedSiblingInstruction::call( + &mut invoke_context, + 0, + VM_BASE_ADDRESS.saturating_add(META_OFFSET as u64), + VM_BASE_ADDRESS.saturating_add(META_OFFSET as u64), + VM_BASE_ADDRESS.saturating_add(META_OFFSET as u64), + VM_BASE_ADDRESS.saturating_add(META_OFFSET as u64), + &mut memory_mapping, + &mut result, + ); + assert!(matches!( + result, + ProgramResult::Err(EbpfError::UserError(error)) if error.downcast_ref::().unwrap() == &BpfError::SyscallError( + SyscallError::CopyOverlapping + ), + )); } #[test] @@ -4219,6 +4384,19 @@ mod tests { SyscallError::BadSeeds(PubkeyError::MaxSeedLengthExceeded) ), )); + + assert!(matches!( + call_program_address_common( + &mut invoke_context, + seeds, + &address, + true, + SyscallTryFindProgramAddress::call, + ), + Err(EbpfError::UserError(error)) if error.downcast_ref::().unwrap() == &BpfError::SyscallError( + SyscallError::CopyOverlapping + ), + )); } #[test] diff --git a/sdk/program/src/program_stubs.rs b/sdk/program/src/program_stubs.rs index 945f6c181..649f20ed4 100644 --- a/sdk/program/src/program_stubs.rs +++ b/sdk/program/src/program_stubs.rs @@ -57,7 +57,7 @@ pub trait SyscallStubs: Sync + Send { unsafe fn sol_memcpy(&self, dst: *mut u8, src: *const u8, n: usize) { // cannot be overlapping assert!( - is_nonoverlapping(src as usize, dst as usize, n), + is_nonoverlapping(src as usize, n, dst as usize, n), "memcpy does not support overlapping regions" ); std::ptr::copy_nonoverlapping(src, dst, n as usize); @@ -207,18 +207,20 @@ pub(crate) fn sol_set_account_properties(updates: &[AccountPropertyUpdate]) { /// Check that two regions do not overlap. /// -/// Adapted from libcore, hidden to share with bpf_loader without being part of -/// the API surface. +/// 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 +pub fn is_nonoverlapping(src: N, src_len: N, dst: N, dst_len: 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, + // If the absolute distance between the ptrs is at least as big as the size of the other, // they do not overlap. - diff >= count + if src > dst { + src - dst >= dst_len + } else { + dst - src >= src_len + } } #[cfg(test)] @@ -227,12 +229,16 @@ mod tests { #[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)); + for dst in 0..8 { + assert!(is_nonoverlapping(10, 3, dst, 3)); + } + for dst in 8..13 { + assert!(!is_nonoverlapping(10, 3, dst, 3)); + } + for dst in 13..20 { + assert!(is_nonoverlapping(10, 3, dst, 3)); + } + assert!(is_nonoverlapping::(255, 3, 254, 1)); + assert!(!is_nonoverlapping::(255, 2, 254, 3)); } } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 8671ee371..89cedb0b3 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -530,6 +530,10 @@ pub mod limit_max_instruction_trace_length { solana_sdk::declare_id!("GQALDaC48fEhZGWRj9iL5Q889emJKcj3aCvHF7VCbbF4"); } +pub mod check_syscall_outputs_do_not_overlap { + solana_sdk::declare_id!("3uRVPBpyEJRo1emLCrq38eLRFGcu6uKSpUXqGvU8T7SZ"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -656,7 +660,8 @@ lazy_static! { (epoch_accounts_hash::id(), "enable epoch accounts hash calculation #27539"), (remove_deprecated_request_unit_ix::id(), "remove support for RequestUnitsDeprecated instruction #27500"), (increase_tx_account_lock_limit::id(), "increase tx account lock limit to 128 #27241"), - (limit_max_instruction_trace_length::id(), "limit max instruction trace length"), + (limit_max_instruction_trace_length::id(), "limit max instruction trace length #27939"), + (check_syscall_outputs_do_not_overlap::id(), "check syscall outputs do_not overlap #28600"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()