From ad813b741790a894ca886145b8c1514773e71aef Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Fri, 7 Jul 2023 14:36:13 +0700 Subject: [PATCH] cpi: update all account perms at once when direct mapping is on When direct mapping is on, we must update all perms at once before doing account data updates. Otherwise, updating an account might write into another account whose perms we haven't updated yet. Tested in TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE. --- programs/bpf_loader/src/syscalls/cpi.rs | 189 ++++++++++++++++------ programs/sbf/rust/invoke/src/processor.rs | 12 +- programs/sbf/tests/programs.rs | 69 ++++---- 3 files changed, 192 insertions(+), 78 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 4d57f88b485c20..0dba90b776c738 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -346,6 +346,19 @@ impl<'a, 'b> CallerAccount<'a, 'b> { rent_epoch: account_info.rent_epoch, }) } + + fn realloc_region( + &self, + memory_mapping: &'b MemoryMapping<'_>, + is_loader_deprecated: bool, + ) -> Result, Error> { + account_realloc_region( + memory_mapping, + self.vm_data_addr, + self.original_data_len, + is_loader_deprecated, + ) + } } type TranslatedAccounts<'a, 'b> = Vec<(IndexOfAccount, Option>)>; @@ -1093,6 +1106,25 @@ fn cpi_common( .feature_set .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + // When direct mapping is on, we must update all perms at once before doing + // account data updates. Otherwise, updating an account might write into + // another account whose perms we haven't updated yet. See + // TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE. + if direct_mapping { + for (index_in_caller, caller_account) in accounts.iter_mut() { + if let Some(caller_account) = caller_account { + let callee_account = instruction_context + .try_borrow_instruction_account(transaction_context, *index_in_caller)?; + update_caller_account_perms( + memory_mapping, + caller_account, + &callee_account, + is_loader_deprecated, + )?; + } + } + } + for (index_in_caller, caller_account) in accounts.iter_mut() { if let Some(caller_account) = caller_account { let mut callee_account = instruction_context @@ -1224,6 +1256,72 @@ fn update_callee_account( Ok(()) } +fn update_caller_account_perms<'a>( + memory_mapping: &'a MemoryMapping, + caller_account: &CallerAccount, + callee_account: &BorrowedAccount<'_>, + is_loader_deprecated: bool, +) -> Result<(Option<&'a MemoryRegion>, Option<&'a MemoryRegion>), Error> { + update_account_regions_perms( + memory_mapping, + caller_account.vm_data_addr, + caller_account.original_data_len, + callee_account, + is_loader_deprecated, + ) +} + +fn update_account_regions_perms<'a>( + memory_mapping: &'a MemoryMapping, + vm_data_addr: u64, + original_data_len: usize, + callee_account: &BorrowedAccount<'_>, + is_loader_deprecated: bool, +) -> Result<(Option<&'a MemoryRegion>, Option<&'a MemoryRegion>), Error> { + let data_region = account_data_region(memory_mapping, vm_data_addr, original_data_len)?; + if let Some(region) = data_region { + match ( + region.state.get(), + callee_account.can_data_be_changed().is_ok(), + ) { + (MemoryState::Readable, true) => { + // If the account is still shared it means it wasn't written to yet during this + // transaction. We map it as CoW and it'll be copied the first time something + // tries to write into it. + if callee_account.is_shared() { + let index_in_transaction = callee_account.get_index_in_transaction(); + region + .state + .set(MemoryState::Cow(index_in_transaction as u64)); + } else { + region.state.set(MemoryState::Writable); + } + } + + (MemoryState::Writable | MemoryState::Cow(_), false) => { + region.state.set(MemoryState::Readable); + } + _ => {} + } + } + let realloc_region = account_realloc_region( + memory_mapping, + vm_data_addr, + original_data_len, + is_loader_deprecated, + )?; + if let Some(region) = realloc_region { + region + .state + .set(if callee_account.can_data_be_changed().is_ok() { + MemoryState::Writable + } else { + MemoryState::Readable + }); + } + Ok((data_region, realloc_region)) +} + // Update the given account after executing CPI. // // caller_account and callee_account describe to the same account. At CPI exit @@ -1244,7 +1342,11 @@ fn update_caller_account( *caller_account.owner = *callee_account.get_owner(); if direct_mapping { - if caller_account.original_data_len > 0 { + if let Some(region) = account_data_region( + memory_mapping, + caller_account.vm_data_addr, + caller_account.original_data_len, + )? { // Since each instruction account is directly mapped in a memory region // with a *fixed* length, upon returning from CPI we must ensure that the // current capacity is at least the original length (what is mapped in @@ -1255,12 +1357,6 @@ fn update_caller_account( callee_account.reserve(min_capacity.saturating_sub(callee_account.capacity()))? } - // We can trust vm_data_addr to point to the correct region because we - // enforce that in CallerAccount::from_(sol_)account_info. - let region = memory_mapping.region(AccessType::Load, caller_account.vm_data_addr)?; - // vm_data_addr must always point to the beginning of the region - debug_assert_eq!(region.vm_addr, caller_account.vm_data_addr); - // If an account's data pointer has changed - because of CoW or because // of using AccountSharedData directly (deprecated) - we must update the // corresponding MemoryRegion in the caller's address space. Address @@ -1270,39 +1366,6 @@ fn update_caller_account( if region.host_addr.get() != callee_ptr { region.host_addr.set(callee_ptr); } - - match ( - region.state.get(), - callee_account.can_data_be_changed().is_ok(), - ) { - (MemoryState::Readable, true) => { - // If the account is still shared it means it wasn't written to yet during this - // transaction. We map it as CoW and it'll be copied the first time something - // tries to write into it. - if callee_account.is_shared() { - let index_in_transaction = callee_account.get_index_in_transaction(); - region - .state - .set(MemoryState::Cow(index_in_transaction as u64)); - } else { - region.state.set(MemoryState::Writable); - } - } - - (MemoryState::Writable | MemoryState::Cow(_), false) => { - region.state.set(MemoryState::Readable); - } - _ => {} - } - } - if !is_loader_deprecated { - realloc_region(caller_account, memory_mapping)?.state.set( - if callee_account.can_data_be_changed().is_ok() { - MemoryState::Writable - } else { - MemoryState::Readable - }, - ); } } @@ -1368,9 +1431,14 @@ 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 { + // If we get here and prev_len > original_data_len, then + // we've already returned InvalidRealloc for the + // bpf_loader_deprecated case. debug_assert!(!is_loader_deprecated); let _guard = { - let realloc_region = realloc_region(caller_account, memory_mapping)?; + let realloc_region = caller_account + .realloc_region(memory_mapping, is_loader_deprecated)? + .unwrap(); // unwrapping here is fine, we already asserted !is_loader_deprecated let state = realloc_region.state.replace(MemoryState::Writable); ReallocStateDropGuard { @@ -1462,7 +1530,9 @@ fn update_caller_account( // 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 realloc_region = caller_account + .realloc_region(memory_mapping, is_loader_deprecated)? + .unwrap(); // unwrapping here is fine, we asserted !is_loader_deprecated let state = realloc_region.state.replace(MemoryState::Writable); ReallocStateDropGuard { @@ -1494,19 +1564,40 @@ fn update_caller_account( Ok(()) } -fn realloc_region<'a>( - caller_account: &CallerAccount<'_, '_>, +fn account_data_region<'a>( 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); + vm_data_addr: u64, + original_data_len: usize, +) -> Result, Error> { + if original_data_len > 0 { + // We can trust vm_data_addr to point to the correct region because we + // enforce that in CallerAccount::from_(sol_)account_info. + let region = memory_mapping.region(AccessType::Load, vm_data_addr)?; + // vm_data_addr must always point to the beginning of the region + debug_assert_eq!(region.vm_addr, vm_data_addr); + return Ok(Some(region)); + } + + Ok(None) +} + +fn account_realloc_region<'a>( + memory_mapping: &'a MemoryMapping<'_>, + vm_data_addr: u64, + original_data_len: usize, + is_loader_deprecated: bool, +) -> Result, Error> { + if is_loader_deprecated { + return Ok(None); + } + + let realloc_vm_addr = vm_data_addr.saturating_add(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) + Ok(Some(realloc_region)) } struct ReallocStateDropGuard<'a> { diff --git a/programs/sbf/rust/invoke/src/processor.rs b/programs/sbf/rust/invoke/src/processor.rs index bf72df01e0f2fb..bc2c20ed091869 100644 --- a/programs/sbf/rust/invoke/src/processor.rs +++ b/programs/sbf/rust/invoke/src/processor.rs @@ -829,7 +829,7 @@ fn process_instruction( 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 }; + unsafe { *serialized_len_ptr = rc_box_size 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, @@ -848,13 +848,17 @@ fn process_instruction( const INVOKE_PROGRAM_INDEX: usize = 3; const REALLOC_PROGRAM_INDEX: usize = 4; let account = &accounts[ARGUMENT_INDEX]; + let target_account_index = instruction_data[1] as usize; + let target_account = &accounts[target_account_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); + target_account.realloc(0, false).unwrap(); + target_account.assign(realloc_program_id); let rc_box_addr = - account.data.borrow_mut().as_mut_ptr() as *mut RcBox>; + target_account.data.borrow_mut().as_mut_ptr() as *mut RcBox>; let rc_box_size = mem::size_of::>>(); unsafe { std::ptr::write( @@ -877,6 +881,8 @@ fn process_instruction( ); } + let serialized_len_ptr = + unsafe { account.data.borrow_mut().as_mut_ptr().offset(-8) as *mut u64 }; // Place a RcBox> in the account data. This // allows us to test having CallerAccount::ref_to_len_in_vm in an // account region. @@ -898,6 +904,7 @@ fn process_instruction( *realloc_program_id, &[ (accounts[ARGUMENT_INDEX].key, true, false), + (target_account.key, true, false), (realloc_program_id, false, false), (invoke_program_id, false, false), ], @@ -907,6 +914,7 @@ fn process_instruction( ) .unwrap(); + unsafe { *serialized_len_ptr = rc_box_size 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 diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 5a827cd1f7f979..b9580ee6d009bd 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -4112,34 +4112,49 @@ fn test_cpi_account_ownership_writability() { } ); - // 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 - ) + // We're going to try and make CPI write ref_to_len_in_vm into a 2nd + // account, so we add an extra one here. + let account2_keypair = Keypair::new(); + let mut account_metas = account_metas.clone(); + account_metas.push(AccountMeta::new(account2_keypair.pubkey(), false)); + + for target_account in [1, account_metas.len() as u8 - 1] { + // 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 account = AccountSharedData::new(42, 0, &invoke_program_id); + bank.store_account(&account2_keypair.pubkey(), &account); + let instruction_data = vec![ + TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE, + target_account, + 42, + 42, + ]; + let instruction = Instruction::new_with_bytes( + invoke_program_id, + &instruction_data, + account_metas.clone(), ); - } 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]); + 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]); + } } } }