From 5c5ec1385febfb59655d558bbdec6ebc91002e90 Mon Sep 17 00:00:00 2001 From: Sean Young Date: Tue, 1 Oct 2024 16:26:46 +0100 Subject: [PATCH] Ensure that account info address is not in an account --- programs/bpf_loader/src/syscalls/cpi.rs | 21 ++++++--- programs/sbf/rust/invoke/src/lib.rs | 38 ++++++++++++++++ programs/sbf/rust/invoke_dep/src/lib.rs | 1 + programs/sbf/tests/programs.rs | 60 +++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 6 deletions(-) diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index d4d626e959d8fe..1f4373fd4af514 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -321,12 +321,6 @@ impl<'a, 'b> CallerAccount<'a, 'b> { .saturating_sub(account_info as *const _ as *const u64 as u64); let ref_to_len_in_vm = if direct_mapping { - // In the same vein as the other check_account_info_pointer() checks, we don't lock this - // pointer to a specific address but we don't want it to be inside accounts, or callees - // might be able to write to the pointed memory. - if data_len_vm_addr >= ebpf::MM_INPUT_START { - return Err(SyscallError::InvalidPointer.into()); - } VmValue::VmAddress { vm_addr: data_len_vm_addr, memory_mapping, @@ -804,6 +798,21 @@ fn translate_account_infos<'a, T, F>( where F: Fn(&T) -> u64, { + let direct_mapping = invoke_context + .get_feature_set() + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + + // In the same vein as the other check_account_info_pointer() checks, we don't lock + // this pointer to a specific address but we don't want it to be inside accounts, or + // callees might be able to write to the pointed memory. + if direct_mapping + && account_infos_addr + .saturating_add(account_infos_len.saturating_mul(std::mem::size_of::() as u64)) + >= ebpf::MM_INPUT_START + { + return Err(SyscallError::InvalidPointer.into()); + } + let account_infos = translate_slice::( memory_mapping, account_infos_addr, diff --git a/programs/sbf/rust/invoke/src/lib.rs b/programs/sbf/rust/invoke/src/lib.rs index 182b5dbe7cb945..650ebca75b767f 100644 --- a/programs/sbf/rust/invoke/src/lib.rs +++ b/programs/sbf/rust/invoke/src/lib.rs @@ -1484,6 +1484,44 @@ fn process_instruction<'a>( ) .unwrap(); } + TEST_ACCOUNT_INFO_IN_ACCOUNT => { + msg!("TEST_ACCOUNT_INFO_IN_ACCOUNT"); + + let account_offset = usize::from_le_bytes(instruction_data[1..9].try_into().unwrap()); + + let mut instruction_data = vec![TEST_WRITE_ACCOUNT, 1]; + instruction_data.extend_from_slice(&1u64.to_le_bytes()); + instruction_data.push(1); + + let data = accounts[ARGUMENT_INDEX].data.borrow().as_ptr(); + let len = accounts.len(); + + let account_info_in_account: &mut [AccountInfo] = unsafe { + std::slice::from_raw_parts_mut(data.add(account_offset) as *mut AccountInfo, len) + }; + + unsafe { + std::ptr::copy_nonoverlapping( + accounts.as_ptr(), + account_info_in_account.as_mut_ptr(), + len, + ); + } + + invoke( + &create_instruction( + *program_id, + &[ + (program_id, false, false), + (accounts[1].key, true, false), + (accounts[0].key, false, false), + ], + instruction_data.to_vec(), + ), + account_info_in_account, + ) + .unwrap(); + } _ => panic!("unexpected program data"), } diff --git a/programs/sbf/rust/invoke_dep/src/lib.rs b/programs/sbf/rust/invoke_dep/src/lib.rs index 19acfc1db6fcf9..a12ac3f9096382 100644 --- a/programs/sbf/rust/invoke_dep/src/lib.rs +++ b/programs/sbf/rust/invoke_dep/src/lib.rs @@ -42,6 +42,7 @@ pub const TEST_CPI_CHANGE_ACCOUNT_DATA_MEMORY_ALLOCATION: u8 = 39; pub const TEST_WRITE_ACCOUNT: u8 = 40; pub const TEST_CALLEE_ACCOUNT_UPDATES: u8 = 41; pub const TEST_STACK_HEAP_ZEROED: u8 = 42; +pub const TEST_ACCOUNT_INFO_IN_ACCOUNT: u8 = 43; pub const MINT_INDEX: usize = 0; pub const ARGUMENT_INDEX: usize = 1; diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 19f6b943a51769..0cc236645b9bd4 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -4929,6 +4929,66 @@ fn test_update_callee_account() { } } +#[test] +fn test_account_info_in_account() { + solana_logger::setup(); + + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config(100_123_456_789); + + for direct_mapping in [false, true] { + let mut bank = Bank::new_for_tests(&genesis_config); + let feature_set = Arc::make_mut(&mut bank.feature_set); + // by default test banks have all features enabled, so we only need to + // disable when needed + if !direct_mapping { + feature_set.deactivate(&feature_set::bpf_account_data_direct_mapping::id()); + } + + let (bank, bank_forks) = bank.wrap_with_bank_forks_for_tests(); + let mut bank_client = BankClient::new_shared(bank.clone()); + let authority_keypair = Keypair::new(); + + let (bank, invoke_program_id) = load_upgradeable_program_and_advance_slot( + &mut bank_client, + bank_forks.as_ref(), + &mint_keypair, + &authority_keypair, + "solana_sbf_rust_invoke", + ); + + let account_keypair = Keypair::new(); + + let mint_pubkey = mint_keypair.pubkey(); + + let account_metas = vec![ + AccountMeta::new(mint_pubkey, true), + AccountMeta::new(account_keypair.pubkey(), false), + AccountMeta::new_readonly(invoke_program_id, false), + ]; + + let mut instruction_data = vec![TEST_ACCOUNT_INFO_IN_ACCOUNT]; + instruction_data.extend_from_slice(32usize.to_le_bytes().as_ref()); + + let instruction = + Instruction::new_with_bytes(invoke_program_id, &instruction_data, account_metas); + + let account = AccountSharedData::new(42, 10240, &invoke_program_id); + + bank.store_account(&account_keypair.pubkey(), &account); + + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + if direct_mapping { + assert!(result.is_err()); + } else { + assert!(result.is_ok()); + } + } +} + #[test] fn test_clone_account_data() { // Test cloning account data works as expect with