From fa28958bd69054d1c2348e0a731011e93d44d7af Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Tue, 20 Jun 2023 10:17:34 +0000 Subject: [PATCH] bpf_loader: direct_mapping: return ExternalAccountDataModified Map access violation errors to ExternalAccountDataModified instead of ReadonlyDataModified when modifying an account that is writable but not owned by the current program. Increases compatibility with pre-direct mapping. --- program-runtime/src/invoke_context.rs | 1 + programs/bpf_loader/src/lib.rs | 49 +++++- programs/bpf_loader/src/serialization.rs | 2 + programs/bpf_loader/src/syscalls/cpi.rs | 1 + programs/sbf/tests/programs.rs | 214 +++++++++++++---------- 5 files changed, 165 insertions(+), 102 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index 15b6f87b0181e8..ff932802ce946f 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -152,6 +152,7 @@ pub struct SyscallContext { #[derive(Debug, Clone)] pub struct SerializedAccountMetadata { pub original_data_len: usize, + pub is_writable: bool, pub vm_data_addr: u64, pub vm_key_addr: u64, pub vm_lamports_addr: u64, diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index f922fcbc798d1a..c216c871d1f59b 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -1547,7 +1547,14 @@ fn execute<'a, 'b: 'a>( let log_collector = invoke_context.get_log_collector(); let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; - let program_id = *instruction_context.get_last_program_key(transaction_context)?; + let (program_id, is_loader_deprecated) = { + let program_account = + instruction_context.try_borrow_last_program_account(transaction_context)?; + ( + *program_account.get_key(), + *program_account.get_owner() == bpf_loader_deprecated::id(), + ) + }; #[cfg(any(target_os = "windows", not(target_arch = "x86_64")))] let use_jit = false; #[cfg(all(not(target_os = "windows"), target_arch = "x86_64"))] @@ -1570,11 +1577,23 @@ fn execute<'a, 'b: 'a>( // save the account addresses so in case of AccessViolation below we can // map to InstructionError::ReadonlyDataModified, which is easier to // diagnose from developers - let account_region_addrs = regions + let account_region_addrs = accounts_metadata .iter() - .map(|r| r.vm_addr..r.vm_addr.saturating_add(r.len)) + .map(|m| { + let mut vm_end = m.vm_data_addr.saturating_add(m.original_data_len as u64); + if !is_loader_deprecated { + vm_end = vm_end.saturating_add(MAX_PERMITTED_DATA_INCREASE as u64) + } + (m.vm_data_addr..vm_end, m.is_writable) + }) .collect::>(); - let addr_is_account_data = |addr: u64| account_region_addrs.iter().any(|r| r.contains(&addr)); + let addr_is_account_data = |addr: u64| { + account_region_addrs + .iter() + .find(|(r, _)| r.contains(&addr)) + .map(|(_, is_writable)| (true, *is_writable)) + .unwrap_or((false, false)) + }; let mut create_vm_time = Measure::start("create_vm"); let mut execute_time; @@ -1646,11 +1665,23 @@ fn execute<'a, 'b: 'a>( address, _size, _section_name, - )) if addr_is_account_data(*address) => { - // We can get here if direct_mapping is enabled and a program tries to - // write to a readonly account. Map the error to ReadonlyDataModified so - // it's easier for devs to diagnose what happened. - Box::new(InstructionError::ReadonlyDataModified) + )) => { + let (is_account_data, is_writable) = addr_is_account_data(*address); + if is_account_data { + // We can get here if direct_mapping is enabled and + // a program tries to write to a readonly region. + // Map the error to ReadonlyDataModified if the + // account !is_writable, and to + // ExternalAccountDataModified if the executing + // program doesn't own the account. + Box::new(if is_writable { + InstructionError::ExternalAccountDataModified + } else { + InstructionError::ReadonlyDataModified + }) + } else { + error + } } _ => error, }; diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 72ee3ffea95c5f..0c9d1046c1b4c1 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -358,6 +358,7 @@ fn serialize_parameters_unaligned( vm_lamports_addr, vm_owner_addr, vm_data_addr, + is_writable: account.is_writable(), }); } }; @@ -492,6 +493,7 @@ fn serialize_parameters_aligned( vm_owner_addr, vm_lamports_addr, vm_data_addr, + is_writable: borrowed_account.is_writable(), }); } SerializeAccount::Duplicate(position) => { diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 1d6772eaa9ad73..aa80d23ac12096 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -2770,6 +2770,7 @@ mod tests { vm_lamports_addr: lamports_addr as u64, vm_owner_addr: owner_addr as u64, vm_data_addr: data_addr as u64, + is_writable: self.is_writable, }, ) } diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 93de04f9e3a05a..0a5725576050b3 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -3939,107 +3939,135 @@ fn test_program_sbf_inner_instruction_alignment_checks() { fn test_cpi_account_ownership_writability() { solana_logger::setup(); - let GenesisConfigInfo { - genesis_config, - mint_keypair, - .. - } = create_genesis_config(100_123_456_789); - let mut bank = Bank::new_for_tests(&genesis_config); - bank.feature_set = Arc::new(FeatureSet::all_enabled()); - let bank = Arc::new(bank); - let mut bank_client = BankClient::new_shared(&bank); - - let invoke_program_id = load_program( - &bank_client, - &bpf_loader::id(), - &mint_keypair, - "solana_sbf_rust_invoke", - ); - - let (bank, invoked_program_id) = load_program_and_advance_slot( - &mut bank_client, - &bpf_loader::id(), - &mint_keypair, - "solana_sbf_rust_invoked", - ); + for direct_mapping in [false, true] { + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config(100_123_456_789); + let mut bank = Bank::new_for_tests(&genesis_config); + let mut feature_set = FeatureSet::all_enabled(); + if !direct_mapping { + feature_set.deactivate(&feature_set::bpf_account_data_direct_mapping::id()); + } + bank.feature_set = Arc::new(feature_set); + let bank = Arc::new(bank); + let mut bank_client = BankClient::new_shared(&bank); - let account_keypair = Keypair::new(); + let invoke_program_id = load_program( + &bank_client, + &bpf_loader::id(), + &mint_keypair, + "solana_sbf_rust_invoke", + ); - let mint_pubkey = mint_keypair.pubkey(); - let account_metas = vec![ - AccountMeta::new(mint_pubkey, true), - AccountMeta::new(account_keypair.pubkey(), false), - AccountMeta::new_readonly(invoked_program_id, false), - AccountMeta::new_readonly(invoke_program_id, false), - ]; + let (bank, invoked_program_id) = load_program_and_advance_slot( + &mut bank_client, + &bpf_loader::id(), + &mint_keypair, + "solana_sbf_rust_invoked", + ); - for (account_size, byte_index) in [ - (0, 0), // first realloc byte - (0, MAX_PERMITTED_DATA_INCREASE as u8), // last realloc byte - (2, 0), // first data byte - (2, 1), // last data byte - (2, 3), // first realloc byte - (2, 2 + MAX_PERMITTED_DATA_INCREASE as u8), // last realloc byte - ] { - bank.register_recent_blockhash(&Hash::new_unique()); - let account = AccountSharedData::new(42, account_size, &invoke_program_id); - bank.store_account(&account_keypair.pubkey(), &account); + let account_keypair = Keypair::new(); - let instruction = Instruction::new_with_bytes( - invoke_program_id, - &[ - TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT, - byte_index, - 42, - 42, - ], - account_metas.clone(), - ); - let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); - assert_eq!( - result.unwrap_err().unwrap(), - TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified) - ); + let mint_pubkey = mint_keypair.pubkey(); + let account_metas = vec![ + AccountMeta::new(mint_pubkey, true), + AccountMeta::new(account_keypair.pubkey(), false), + AccountMeta::new_readonly(invoked_program_id, false), + AccountMeta::new_readonly(invoke_program_id, false), + ]; - let instruction = Instruction::new_with_bytes( - invoke_program_id, - &[ - TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_OTHER_PROGRAM, - byte_index, - 42, - 42, - ], - account_metas.clone(), - ); - let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); - assert_eq!( - result.unwrap_err().unwrap(), - TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified) - ); + for (account_size, byte_index) in [ + (0, 0), // first realloc byte + (0, MAX_PERMITTED_DATA_INCREASE as u8), // last realloc byte + (2, 0), // first data byte + (2, 1), // last data byte + (2, 3), // first realloc byte + (2, 2 + MAX_PERMITTED_DATA_INCREASE as u8), // last realloc byte + ] { + bank.register_recent_blockhash(&Hash::new_unique()); + let account = AccountSharedData::new(42, account_size, &invoke_program_id); + bank.store_account(&account_keypair.pubkey(), &account); + + let instruction = Instruction::new_with_bytes( + invoke_program_id, + &[ + TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT, + byte_index, + 42, + 42, + ], + account_metas.clone(), + ); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + if (byte_index as usize) < account_size || direct_mapping { + assert_eq!( + result.unwrap_err().unwrap(), + TransactionError::InstructionError( + 0, + InstructionError::ExternalAccountDataModified + ) + ); + } else { + // without direct mapping, changes to the realloc padding + // outside the account length are ignored + assert!(result.is_ok(), "{result:?}"); + } - // assign the account to invoked_program_id, which will then assign to invoke_program_id - let mut account = bank.get_account(&account_keypair.pubkey()).unwrap(); - account.set_owner(invoked_program_id); - bank.store_account(&account_keypair.pubkey(), &account); + let account = AccountSharedData::new(42, account_size, &invoke_program_id); + bank.store_account(&account_keypair.pubkey(), &account); + let instruction = Instruction::new_with_bytes( + invoke_program_id, + &[ + TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_OTHER_PROGRAM, + byte_index, + 42, + 42, + ], + account_metas.clone(), + ); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + if (byte_index as usize) < account_size || direct_mapping { + assert_eq!( + result.unwrap_err().unwrap(), + TransactionError::InstructionError( + 0, + InstructionError::ExternalAccountDataModified + ) + ); + } else { + // without direct mapping, changes to the realloc padding + // outside the account length are ignored + assert!(result.is_ok(), "{result:?}"); + } - let instruction = Instruction::new_with_bytes( - invoke_program_id, - &[ - TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER, - byte_index, - 42, - 42, - ], - account_metas.clone(), - ); - let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); - assert!(result.is_ok(), "{result:?}"); - let account = bank.get_account(&account_keypair.pubkey()).unwrap(); - let mut data = vec![0; account.data().len()]; - if (byte_index as usize) < data.len() { - data[byte_index as usize] = 42; + // assign the account to invoked_program_id, which will then assign to invoke_program_id + // let mut account = bank.get_account(&account_keypair.pubkey()).unwrap(); + // account.set_owner(invoked_program_id); + // bank.store_account(&account_keypair.pubkey(), &account); + let account = AccountSharedData::new(42, account_size, &invoked_program_id); + bank.store_account(&account_keypair.pubkey(), &account); + + let instruction = Instruction::new_with_bytes( + invoke_program_id, + &[ + TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER, + byte_index, + 42, + 42, + ], + account_metas.clone(), + ); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + assert!(result.is_ok(), "{result:?}"); + let account = bank.get_account(&account_keypair.pubkey()).unwrap(); + let mut data = vec![0; account.data().len()]; + if (byte_index as usize) < data.len() { + data[byte_index as usize] = 42; + } + assert_eq!(account.data(), data); } - assert_eq!(account.data(), data); } }