diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 3581125e0f57ad..25ea1ebf8282c0 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -28,11 +28,44 @@ macro_rules! check_account_info_pointer { }; } +enum VmValue<'a, 'b, T> { + VmAddress { + vm_addr: u64, + memory_mapping: &'b MemoryMapping<'a>, + check_aligned: bool, + }, + Translated(&'a mut T), +} + +impl<'a, 'b, T> VmValue<'a, 'b, T> { + fn get(&self) -> Result<&T, Error> { + match self { + VmValue::VmAddress { + vm_addr, + memory_mapping, + check_aligned, + } => translate_type(memory_mapping, *vm_addr, *check_aligned), + VmValue::Translated(addr) => Ok(*addr), + } + } + + fn get_mut(&mut self) -> Result<&mut T, Error> { + match self { + VmValue::VmAddress { + vm_addr, + memory_mapping, + check_aligned, + } => translate_type_mut(memory_mapping, *vm_addr, *check_aligned), + VmValue::Translated(addr) => Ok(*addr), + } + } +} + /// Host side representation of AccountInfo or SolAccountInfo passed to the CPI syscall. /// /// At the start of a CPI, this can be different from the data stored in the /// corresponding BorrowedAccount, and needs to be synched. -struct CallerAccount<'a> { +struct CallerAccount<'a, 'b> { lamports: &'a mut u64, owner: &'a mut Pubkey, // The original data length of the account at the start of the current @@ -50,22 +83,22 @@ struct CallerAccount<'a> { // Given the corresponding input AccountInfo::data, vm_data_addr points to // the pointer field and ref_to_len_in_vm points to the length field. vm_data_addr: u64, - ref_to_len_in_vm: &'a mut u64, + ref_to_len_in_vm: VmValue<'b, 'a, u64>, // To be removed once `feature_set::move_serialized_len_ptr_in_cpi` is active everywhere serialized_len_ptr: *mut u64, executable: bool, rent_epoch: u64, } -impl<'a> CallerAccount<'a> { +impl<'a, 'b> CallerAccount<'a, 'b> { // Create a CallerAccount given an AccountInfo. fn from_account_info( invoke_context: &InvokeContext, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, _vm_addr: u64, account_info: &AccountInfo, account_metadata: &SerializedAccountMetadata, - ) -> Result, Error> { + ) -> Result, Error> { let direct_mapping = invoke_context .feature_set .is_active(&feature_set::bpf_account_data_direct_mapping::id()); @@ -126,14 +159,23 @@ impl<'a> CallerAccount<'a> { .saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit), )?; - let translated = translate( - memory_mapping, - AccessType::Store, - (account_info.data.as_ptr() as *const u64 as u64) - .saturating_add(size_of::() as u64), - 8, - )? as *mut u64; - let ref_to_len_in_vm = unsafe { &mut *translated }; + let ref_to_len_in_vm = if direct_mapping { + VmValue::VmAddress { + vm_addr: (account_info.data.as_ptr() as *const u64 as u64) + .saturating_add(size_of::() as u64), + memory_mapping, + check_aligned: invoke_context.get_check_aligned(), + } + } else { + let translated = translate( + memory_mapping, + AccessType::Store, + (account_info.data.as_ptr() as *const u64 as u64) + .saturating_add(size_of::() as u64), + 8, + )? as *mut u64; + VmValue::Translated(unsafe { &mut *translated }) + }; let serialized_len_ptr = if invoke_context .feature_set .is_active(&feature_set::move_serialized_len_ptr_in_cpi::id()) @@ -201,7 +243,7 @@ impl<'a> CallerAccount<'a> { vm_addr: u64, account_info: &SolAccountInfo, account_metadata: &SerializedAccountMetadata, - ) -> Result, Error> { + ) -> Result, Error> { let direct_mapping = invoke_context .feature_set .is_active(&feature_set::bpf_account_data_direct_mapping::id()); @@ -275,7 +317,7 @@ impl<'a> CallerAccount<'a> { data_len_vm_addr, size_of::() as u64, )?; - let ref_to_len_in_vm = unsafe { &mut *(data_len_addr as *mut u64) }; + let ref_to_len_in_vm = VmValue::Translated(unsafe { &mut *(data_len_addr as *mut u64) }); let ref_of_len_in_input_buffer = (account_info.data_addr as *mut u8 as u64).saturating_sub(8); @@ -306,7 +348,7 @@ impl<'a> CallerAccount<'a> { } } -type TranslatedAccounts<'a> = Vec<(IndexOfAccount, Option>)>; +type TranslatedAccounts<'a, 'b> = Vec<(IndexOfAccount, Option>)>; /// Implemented by language specific data structure translators trait SyscallInvokeSigned { @@ -315,15 +357,15 @@ trait SyscallInvokeSigned { memory_mapping: &MemoryMapping, invoke_context: &mut InvokeContext, ) -> Result; - fn translate_accounts<'a>( + fn translate_accounts<'a, 'b>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, is_loader_deprecated: bool, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, invoke_context: &mut InvokeContext, - ) -> Result, Error>; + ) -> Result, Error>; fn translate_signers( program_id: &Pubkey, signers_seeds_addr: u64, @@ -408,15 +450,15 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { }) } - fn translate_accounts<'a>( + fn translate_accounts<'a, 'b>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, is_loader_deprecated: bool, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, invoke_context: &mut InvokeContext, - ) -> Result, Error> { + ) -> Result, Error> { let (account_infos, account_info_keys) = translate_account_infos( account_infos_addr, account_infos_len, @@ -641,15 +683,15 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { }) } - fn translate_accounts<'a>( + fn translate_accounts<'a, 'b>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, is_loader_deprecated: bool, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, invoke_context: &mut InvokeContext, - ) -> Result, Error> { + ) -> Result, Error> { let (account_infos, account_info_keys) = translate_account_infos( account_infos_addr, account_infos_len, @@ -758,7 +800,7 @@ where // Finish translating accounts, build CallerAccount values and update callee // accounts in preparation of executing the callee. -fn translate_and_update_accounts<'a, T, F>( +fn translate_and_update_accounts<'a, 'b, T, F>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_info_keys: &[&Pubkey], @@ -766,17 +808,17 @@ fn translate_and_update_accounts<'a, T, F>( account_infos_addr: u64, is_loader_deprecated: bool, invoke_context: &mut InvokeContext, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, do_translate: F, -) -> Result, Error> +) -> Result, Error> where F: Fn( &InvokeContext, - &MemoryMapping, + &'b MemoryMapping<'a>, u64, &T, &SerializedAccountMetadata, - ) -> Result, Error>, + ) -> Result, Error>, { let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; @@ -1094,7 +1136,7 @@ fn update_callee_account( if direct_mapping { let prev_len = callee_account.get_data().len(); - let post_len = *caller_account.ref_to_len_in_vm as usize; + let post_len = *caller_account.ref_to_len_in_vm.get()? as usize; match callee_account .can_data_be_resized(post_len) .and_then(|_| callee_account.can_data_be_changed()) @@ -1201,7 +1243,7 @@ fn update_caller_account( *caller_account.lamports = callee_account.get_lamports(); *caller_account.owner = *callee_account.get_owner(); - let _realloc_perms_drop = if direct_mapping { + if direct_mapping { if caller_account.original_data_len > 0 { // Since each instruction account is directly mapped in a memory region // with a *fixed* length, upon returning from CPI we must ensure that the @@ -1254,54 +1296,17 @@ fn update_caller_account( } } if !is_loader_deprecated { - // the realloc region starts at the end of the original data - let realloc_vm_addr = caller_account - .vm_data_addr - .saturating_add(caller_account.original_data_len as u64); - let realloc_region = memory_mapping.region(AccessType::Load, realloc_vm_addr)?; - debug_assert_eq!(realloc_region.vm_addr, realloc_vm_addr); - debug_assert!((MAX_PERMITTED_DATA_INCREASE - ..MAX_PERMITTED_DATA_INCREASE.saturating_add(BPF_ALIGN_OF_U128)) - .contains(&(realloc_region.len as usize))); - - // If a callee reallocs an account, we write into the caller's - // realloc region regardless of whether the caller has write - // permissions to the account or not. If the callee has been able to - // make changes, it means they had permissions to do so, and here - // we're just going to reflect those changes to the caller's frame. - // - // Therefore we temporarily configure the realloc region as writable - // for the duration of this function and then if necessary drop - // perms back right at the end. - realloc_region.state.set(MemoryState::Writable); - - struct ReallocDropGuard<'a> { - realloc_region: &'a MemoryRegion, - state: MemoryState, - } - - impl<'a> Drop for ReallocDropGuard<'a> { - fn drop(&mut self) { - self.realloc_region.state.set(self.state); - } - } - - Some(ReallocDropGuard { - realloc_region, - state: if callee_account.can_data_be_changed().is_ok() { + realloc_region(caller_account, memory_mapping)?.state.set( + if callee_account.can_data_be_changed().is_ok() { MemoryState::Writable } else { MemoryState::Readable }, - }) - } else { - None + ); } - } else { - None - }; + } - let prev_len = *caller_account.ref_to_len_in_vm as usize; + let prev_len = *caller_account.ref_to_len_in_vm.get()? as usize; let post_len = callee_account.get_data().len(); let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); if prev_len != post_len { @@ -1363,6 +1368,16 @@ fn update_caller_account( // or it must zero the account data, therefore making the // zeroing we do here redundant. if prev_len > caller_account.original_data_len { + debug_assert!(!is_loader_deprecated); + let _guard = { + let realloc_region = realloc_region(caller_account, memory_mapping)?; + let state = realloc_region.state.replace(MemoryState::Writable); + + ReallocStateDropGuard { + realloc_region, + state, + } + }; // We need to zero the unused space in the realloc region, // starting after the last byte of the new data which might // be > original_data_len. @@ -1401,7 +1416,7 @@ fn update_caller_account( )?; } // this is the len field in the AccountInfo::data slice - *caller_account.ref_to_len_in_vm = post_len as u64; + *caller_account.ref_to_len_in_vm.get_mut()? = post_len as u64; // this is the len field in the serialized parameters if invoke_context @@ -1436,15 +1451,36 @@ fn update_caller_account( // In the is_loader_deprecated case, we must have failed with // InvalidRealloc by now. debug_assert!(!is_loader_deprecated); - let to_slice = translate_slice_mut::( - memory_mapping, - caller_account - .vm_data_addr - .saturating_add(caller_account.original_data_len as u64), - realloc_bytes_used as u64, - invoke_context.get_check_aligned(), - invoke_context.get_check_size(), - )?; + + let to_slice = { + // If a callee reallocs an account, we write into the caller's + // realloc region regardless of whether the caller has write + // permissions to the account or not. If the callee has been able to + // make changes, it means they had permissions to do so, and here + // we're just going to reflect those changes to the caller's frame. + // + // Therefore we temporarily configure the realloc region as writable + // then set it back to whatever state it had. + let _guard = { + let realloc_region = realloc_region(caller_account, memory_mapping)?; + let state = realloc_region.state.replace(MemoryState::Writable); + + ReallocStateDropGuard { + realloc_region, + state, + } + }; + + translate_slice_mut::( + memory_mapping, + caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64), + realloc_bytes_used as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )? + }; let from_slice = callee_account .get_data() .get(caller_account.original_data_len..post_len) @@ -1458,6 +1494,32 @@ fn update_caller_account( Ok(()) } +fn realloc_region<'a>( + caller_account: &CallerAccount<'_, '_>, + memory_mapping: &'a MemoryMapping<'_>, +) -> Result<&'a MemoryRegion, Error> { + let realloc_vm_addr = caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64); + let realloc_region = memory_mapping.region(AccessType::Load, realloc_vm_addr)?; + debug_assert_eq!(realloc_region.vm_addr, realloc_vm_addr); + debug_assert!((MAX_PERMITTED_DATA_INCREASE + ..MAX_PERMITTED_DATA_INCREASE.saturating_add(BPF_ALIGN_OF_U128)) + .contains(&(realloc_region.len as usize))); + Ok(realloc_region) +} + +struct ReallocStateDropGuard<'a> { + realloc_region: &'a MemoryRegion, + state: MemoryState, +} + +impl<'a> Drop for ReallocStateDropGuard<'a> { + fn drop(&mut self) { + self.realloc_region.state.set(self.state); + } +} + #[allow(clippy::indexing_slicing)] #[allow(clippy::integer_arithmetic)] #[cfg(test)] @@ -1659,7 +1721,7 @@ mod tests { assert_eq!(caller_account.owner, account.owner()); assert_eq!(caller_account.original_data_len, account.data().len()); assert_eq!( - *caller_account.ref_to_len_in_vm as usize, + *caller_account.ref_to_len_in_vm.get().unwrap() as usize, account.data().len() ); assert_eq!(caller_account.serialized_data, account.data()); @@ -1787,7 +1849,10 @@ mod tests { .unwrap(); let data_len = callee_account.get_data().len(); - assert_eq!(data_len, *caller_account.ref_to_len_in_vm as usize); + assert_eq!( + data_len, + *caller_account.ref_to_len_in_vm.get().unwrap() as usize + ); assert_eq!(data_len, serialized_len()); assert_eq!(data_len, caller_account.serialized_data.len()); assert_eq!( @@ -1932,7 +1997,10 @@ mod tests { let data_len = callee_account.get_data().len(); // the account info length must get updated - assert_eq!(data_len, *caller_account.ref_to_len_in_vm as usize); + assert_eq!( + data_len, + *caller_account.ref_to_len_in_vm.get().unwrap() as usize + ); // the length slot in the serialization parameters must be updated assert_eq!(data_len, serialized_len()); @@ -2211,7 +2279,7 @@ mod tests { // close the account let mut data = Vec::new(); caller_account.serialized_data = &mut data; - *caller_account.ref_to_len_in_vm = 0; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = 0; let mut owner = system_program::id(); caller_account.owner = &mut owner; update_callee_account( @@ -2280,7 +2348,7 @@ mod tests { // without direct mapping let mut data = b"foobarbaz".to_vec(); - *caller_account.ref_to_len_in_vm = data.len() as u64; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = data.len() as u64; caller_account.serialized_data = &mut data; let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2298,7 +2366,7 @@ mod tests { // with direct mapping let mut data = b"baz".to_vec(); - *caller_account.ref_to_len_in_vm = 9; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = 9; caller_account.serialized_data = &mut data; let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2375,7 +2443,7 @@ mod tests { (6, b"foobar".to_vec()), // == original_data_len, truncates (3, b"foo".to_vec()), // < original_data_len, truncates ] { - *caller_account.ref_to_len_in_vm = len as u64; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = len as u64; update_callee_account( &invoke_context, &memory_mapping, @@ -2390,7 +2458,7 @@ mod tests { } // close the account - *caller_account.ref_to_len_in_vm = 0; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = 0; let mut owner = system_program::id(); caller_account.owner = &mut owner; update_callee_account( @@ -2550,7 +2618,7 @@ mod tests { } } - fn caller_account(&mut self) -> CallerAccount<'_> { + fn caller_account(&mut self) -> CallerAccount<'_, '_> { let data = if self.direct_mapping { &mut [] } else { @@ -2562,7 +2630,7 @@ mod tests { original_data_len: self.len as usize, serialized_data: data, vm_data_addr: self.vm_addr + mem::size_of::() as u64, - ref_to_len_in_vm: &mut self.len, + ref_to_len_in_vm: VmValue::Translated(&mut self.len), serialized_len_ptr: std::ptr::null_mut(), executable: false, rent_epoch: 0, diff --git a/programs/sbf/rust/deprecated_loader/src/lib.rs b/programs/sbf/rust/deprecated_loader/src/lib.rs index 75efadddda061a..ca0965c63c999d 100644 --- a/programs/sbf/rust/deprecated_loader/src/lib.rs +++ b/programs/sbf/rust/deprecated_loader/src/lib.rs @@ -17,8 +17,8 @@ use solana_program::{ pub const REALLOC: u8 = 1; pub const REALLOC_EXTEND_FROM_SLICE: u8 = 12; -pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS: u8 = 26; -pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_NESTED: u8 = 27; +pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS: u8 = 28; +pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_NESTED: u8 = 29; #[derive(Debug, PartialEq)] struct SStruct { diff --git a/programs/sbf/rust/invoke/src/instructions.rs b/programs/sbf/rust/invoke/src/instructions.rs index 49d2b7b6796b72..323a15392b0b99 100644 --- a/programs/sbf/rust/invoke/src/instructions.rs +++ b/programs/sbf/rust/invoke/src/instructions.rs @@ -24,17 +24,19 @@ pub const TEST_MAX_ACCOUNT_INFOS_EXCEEDED: u8 = 21; pub const TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT: u8 = 22; pub const TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT_NESTED: u8 = 23; pub const TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_OTHER_PROGRAM: u8 = 24; -pub const TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER: u8 = 25; -pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS: u8 = 26; -pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_NESTED: u8 = 27; -pub const TEST_CPI_ACCOUNT_UPDATE_CALLEE_GROWS: u8 = 28; -pub const TEST_CPI_ACCOUNT_UPDATE_CALLEE_SHRINKS_SMALLER_THAN_ORIGINAL_LEN: u8 = 29; -pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS: u8 = 30; -pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS_NESTED: u8 = 31; -pub const TEST_CPI_INVALID_KEY_POINTER: u8 = 32; -pub const TEST_CPI_INVALID_OWNER_POINTER: u8 = 33; -pub const TEST_CPI_INVALID_LAMPORTS_POINTER: u8 = 34; -pub const TEST_CPI_INVALID_DATA_POINTER: u8 = 35; +pub const TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE_MOVING_DATA_POINTER: u8 = 25; +pub const TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE: u8 = 26; +pub const TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER: u8 = 27; +pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS: u8 = 28; +pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_NESTED: u8 = 29; +pub const TEST_CPI_ACCOUNT_UPDATE_CALLEE_GROWS: u8 = 30; +pub const TEST_CPI_ACCOUNT_UPDATE_CALLEE_SHRINKS_SMALLER_THAN_ORIGINAL_LEN: u8 = 31; +pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS: u8 = 32; +pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS_NESTED: u8 = 33; +pub const TEST_CPI_INVALID_KEY_POINTER: u8 = 34; +pub const TEST_CPI_INVALID_OWNER_POINTER: u8 = 35; +pub const TEST_CPI_INVALID_LAMPORTS_POINTER: u8 = 36; +pub const TEST_CPI_INVALID_DATA_POINTER: u8 = 37; pub const MINT_INDEX: usize = 0; pub const ARGUMENT_INDEX: usize = 1; diff --git a/programs/sbf/rust/invoke/src/processor.rs b/programs/sbf/rust/invoke/src/processor.rs index 3e3398ee3aaf47..bf72df01e0f2fb 100644 --- a/programs/sbf/rust/invoke/src/processor.rs +++ b/programs/sbf/rust/invoke/src/processor.rs @@ -22,7 +22,7 @@ use { }, solana_sbf_rust_invoked::instructions::*, solana_sbf_rust_realloc::instructions::*, - std::{mem, slice}, + std::{cell::RefCell, mem, rc::Rc, slice}, }; fn do_nested_invokes(num_nested_invokes: u64, accounts: &[AccountInfo]) -> ProgramResult { @@ -754,6 +754,171 @@ fn process_instruction( .get_unchecked_mut(instruction_data[1] as usize) = 42 }; } + TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE_MOVING_DATA_POINTER => { + msg!("TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE_MOVING_DATA_POINTER"); + const INVOKE_PROGRAM_INDEX: usize = 3; + const REALLOC_PROGRAM_INDEX: usize = 4; + let account = &accounts[ARGUMENT_INDEX]; + let realloc_program_id = accounts[REALLOC_PROGRAM_INDEX].key; + let invoke_program_id = accounts[INVOKE_PROGRAM_INDEX].key; + account.realloc(0, false).unwrap(); + account.assign(realloc_program_id); + + // Place a RcBox> in the account data. This + // allows us to test having CallerAccount::ref_to_len_in_vm in an + // account region. + let rc_box_addr = + account.data.borrow_mut().as_mut_ptr() as *mut RcBox>; + let rc_box_size = mem::size_of::>>(); + unsafe { + std::ptr::write( + rc_box_addr, + RcBox { + strong: 1, + weak: 0, + // We're testing what happens if we make CPI update the + // slice length after we put the slice in the account + // address range. To do so, we need to move the data + // pointer past the RcBox or CPI will clobber the length + // change when it copies the callee's account data back + // into the caller's account data + // https://github.com/solana-labs/solana/blob/fa28958bd69054d1c2348e0a731011e93d44d7af/programs/bpf_loader/src/syscalls/cpi.rs#L1487 + value: RefCell::new(slice::from_raw_parts_mut( + account.data.borrow_mut().as_mut_ptr().add(rc_box_size), + 0, + )), + }, + ); + } + + // CPI now will update the serialized length in the wrong place, + // since we moved the account data slice. To hit the corner case we + // want to hit, we'll need to update the serialized length manually + // or during deserialize_parameters() we'll get + // AccountDataSizeChanged + let serialized_len_ptr = + unsafe { account.data.borrow_mut().as_mut_ptr().offset(-8) as *mut u64 }; + unsafe { + std::ptr::write( + &account.data as *const _ as usize as *mut Rc>, + Rc::from_raw(((rc_box_addr as usize) + mem::size_of::() * 2) as *mut _), + ); + } + + let mut instruction_data = vec![REALLOC, 0]; + instruction_data.extend_from_slice(&rc_box_size.to_le_bytes()); + + // check that the account is empty before we CPI + assert_eq!(account.data_len(), 0); + + invoke( + &create_instruction( + *realloc_program_id, + &[ + (accounts[ARGUMENT_INDEX].key, true, false), + (realloc_program_id, false, false), + (invoke_program_id, false, false), + ], + instruction_data.to_vec(), + ), + accounts, + ) + .unwrap(); + + // verify that CPI did update `ref_to_len_in_vm` + assert_eq!(account.data_len(), rc_box_size); + + // update the serialized length so we don't error out early with AccountDataSizeChanged + unsafe { *serialized_len_ptr = account.data_len() as u64 }; + + // hack to avoid dropping the RcBox, which is supposed to be on the + // heap but we put it into account data. If we don't do this, + // dropping the Rc will cause + // global_deallocator.dealloc(rc_box_addr) which is invalid and + // happens to write a poison value into the account. + unsafe { + std::ptr::write( + &account.data as *const _ as usize as *mut Rc>, + Rc::new(RefCell::new(&mut [])), + ); + } + } + TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE => { + msg!("TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE"); + const INVOKE_PROGRAM_INDEX: usize = 3; + const REALLOC_PROGRAM_INDEX: usize = 4; + let account = &accounts[ARGUMENT_INDEX]; + let realloc_program_id = accounts[REALLOC_PROGRAM_INDEX].key; + let invoke_program_id = accounts[INVOKE_PROGRAM_INDEX].key; + account.realloc(0, false).unwrap(); + account.assign(realloc_program_id); + + let rc_box_addr = + account.data.borrow_mut().as_mut_ptr() as *mut RcBox>; + let rc_box_size = mem::size_of::>>(); + unsafe { + std::ptr::write( + rc_box_addr, + RcBox { + strong: 1, + weak: 0, + // The difference with + // TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE_MOVING_DATA_POINTER + // is that we don't move the data pointer past the + // RcBox. This is needed to avoid the "Invalid account + // info pointer" check when direct mapping is enabled. + // This also means we don't need to update the + // serialized len like we do in the other test. + value: RefCell::new(slice::from_raw_parts_mut( + account.data.borrow_mut().as_mut_ptr(), + 0, + )), + }, + ); + } + + // Place a RcBox> in the account data. This + // allows us to test having CallerAccount::ref_to_len_in_vm in an + // account region. + unsafe { + std::ptr::write( + &account.data as *const _ as usize as *mut Rc>, + Rc::from_raw(((rc_box_addr as usize) + mem::size_of::() * 2) as *mut _), + ); + } + + let mut instruction_data = vec![REALLOC, 0]; + instruction_data.extend_from_slice(&rc_box_size.to_le_bytes()); + + // check that the account is empty before we CPI + assert_eq!(account.data_len(), 0); + + invoke( + &create_instruction( + *realloc_program_id, + &[ + (accounts[ARGUMENT_INDEX].key, true, false), + (realloc_program_id, false, false), + (invoke_program_id, false, false), + ], + instruction_data.to_vec(), + ), + accounts, + ) + .unwrap(); + + // hack to avoid dropping the RcBox, which is supposed to be on the + // heap but we put it into account data. If we don't do this, + // dropping the Rc will cause + // global_deallocator.dealloc(rc_box_addr) which is invalid and + // happens to write a poison value into the account. + unsafe { + std::ptr::write( + &account.data as *const _ as usize as *mut Rc>, + Rc::new(RefCell::new(&mut [])), + ); + } + } TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER => { msg!("TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER"); const INVOKE_PROGRAM_INDEX: usize = 3; @@ -1051,3 +1216,10 @@ fn process_instruction( Ok(()) } + +#[repr(C)] +struct RcBox { + strong: usize, + weak: usize, + value: T, +} diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 3551b73e766926..7bf3f1f3a040bb 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -3962,13 +3962,20 @@ fn test_cpi_account_ownership_writability() { "solana_sbf_rust_invoke", ); - let (bank, invoked_program_id) = load_program_and_advance_slot( - &mut bank_client, + let invoked_program_id = load_program( + &bank_client, &bpf_loader::id(), &mint_keypair, "solana_sbf_rust_invoked", ); + let (bank, realloc_program_id) = load_program_and_advance_slot( + &mut bank_client, + &bpf_loader::id(), + &mint_keypair, + "solana_sbf_rust_realloc", + ); + let account_keypair = Keypair::new(); let mint_pubkey = mint_keypair.pubkey(); @@ -3977,6 +3984,7 @@ fn test_cpi_account_ownership_writability() { AccountMeta::new(account_keypair.pubkey(), false), AccountMeta::new_readonly(invoked_program_id, false), AccountMeta::new_readonly(invoke_program_id, false), + AccountMeta::new_readonly(realloc_program_id, false), ]; for (account_size, byte_index) in [ @@ -4069,6 +4077,71 @@ fn test_cpi_account_ownership_writability() { } assert_eq!(account.data(), data); } + + // Test that the CPI code that updates `ref_to_len_in_vm` fails if we + // make it write to an invalid location. This is the first variant which + // correctly triggers ExternalAccountDataModified when direct mapping is + // disabled. When direct mapping is enabled this tests fails early + // because we move the account data pointer. + // TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE is able to make more + // progress when direct mapping is on. + let account = AccountSharedData::new(42, 0, &invoke_program_id); + bank.store_account(&account_keypair.pubkey(), &account); + let instruction_data = vec![ + TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE_MOVING_DATA_POINTER, + 42, + 42, + 42, + ]; + let instruction = Instruction::new_with_bytes( + invoke_program_id, + &instruction_data, + account_metas.clone(), + ); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + assert_eq!( + result.unwrap_err().unwrap(), + if direct_mapping { + // We move the data pointer, direct mapping doesn't allow it + // anymore so it errors out earlier. See + // test_cpi_invalid_account_info_pointers. + TransactionError::InstructionError(0, InstructionError::ProgramFailedToComplete) + } else { + // We managed to make CPI write into the account data, but the + // usual checks still apply and we get an error. + TransactionError::InstructionError(0, InstructionError::ExternalAccountDataModified) + } + ); + + // Similar to the test above where we try to make CPI write into account + // data. This variant is for when direct mapping is enabled. + let account = AccountSharedData::new(42, 0, &invoke_program_id); + bank.store_account(&account_keypair.pubkey(), &account); + let instruction_data = vec![TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE, 42, 42, 42]; + let instruction = Instruction::new_with_bytes( + invoke_program_id, + &instruction_data, + account_metas.clone(), + ); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + if direct_mapping { + assert_eq!( + result.unwrap_err().unwrap(), + TransactionError::InstructionError( + 0, + InstructionError::ExternalAccountDataModified + ) + ); + } else { + // we expect this to succeed as after updating `ref_to_len_in_vm`, + // CPI will sync the actual account data between the callee and the + // caller, _always_ writing over the location pointed by + // `ref_to_len_in_vm`. To verify this, we check that the account + // data is in fact all zeroes like it is in the callee. + result.unwrap(); + let account = bank.get_account(&account_keypair.pubkey()).unwrap(); + assert_eq!(account.data(), vec![0; 40]); + } } } @@ -4149,7 +4222,7 @@ fn test_cpi_account_data_updates() { account_metas.clone(), ); let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); - assert!(result.is_ok(), "{result:?}"); + result.unwrap(); let account = bank.get_account(&account_keypair.pubkey()).unwrap(); // "bar" here was copied from the realloc region assert_eq!(account.data(), b"foobar");