From 954848a0c9cd272d4b320c79967b4c998f4c97a3 Mon Sep 17 00:00:00 2001 From: HaoranYi Date: Tue, 21 Nov 2023 15:21:33 +0000 Subject: [PATCH] use PROGRAM_OWNER + program data to decide whether an account is executable during transaction loading and processing --- program-runtime/src/invoke_context.rs | 6 +-- programs/bpf_loader/src/lib.rs | 45 +-------------------- runtime/src/accounts/mod.rs | 51 ++++++++++++++++++----- runtime/src/bank.rs | 12 +++--- sdk/src/account.rs | 58 ++++++++++++++++++++++++++- sdk/src/transaction_context.rs | 15 +++++-- 6 files changed, 121 insertions(+), 66 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index bdb870a02c1dda..534219fb12c8a1 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -962,8 +962,8 @@ mod tests { let owned_account = AccountSharedData::new(42, 1, &callee_program_id); let not_owned_account = AccountSharedData::new(84, 1, &solana_sdk::pubkey::new_rand()); let readonly_account = AccountSharedData::new(168, 1, &solana_sdk::pubkey::new_rand()); - let loader_account = AccountSharedData::new(0, 0, &native_loader::id()); - let mut program_account = AccountSharedData::new(1, 0, &native_loader::id()); + let loader_account = AccountSharedData::new(0, 1, &native_loader::id()); + let mut program_account = AccountSharedData::new(1, 1, &native_loader::id()); program_account.set_executable(true); let transaction_accounts = vec![ (solana_sdk::pubkey::new_rand(), owned_account), @@ -990,7 +990,7 @@ mod tests { let mut programs_loaded_for_tx_batch = LoadedProgramsForTxBatch::default(); programs_loaded_for_tx_batch.replenish( callee_program_id, - Arc::new(LoadedProgram::new_builtin(0, 0, MockBuiltin::vm)), + Arc::new(LoadedProgram::new_builtin(0, 1, MockBuiltin::vm)), ); invoke_context.programs_loaded_for_tx_batch = &programs_loaded_for_tx_batch; diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 0f9e8304392de6..d8e1eca2def416 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -2641,36 +2641,13 @@ mod tests { &elf_orig, &elf_new, ); - transaction_accounts - .get_mut(1) - .unwrap() - .1 - .set_executable(false); + transaction_accounts.get_mut(1).unwrap().1.set_data(vec![]); // set the account data empty to make it not executable process_instruction( transaction_accounts, instruction_accounts, Err(InstructionError::AccountNotExecutable), ); - // Case: Program account now owned by loader - let (mut transaction_accounts, instruction_accounts) = get_accounts( - &buffer_address, - &upgrade_authority_address, - &upgrade_authority_address, - &elf_orig, - &elf_new, - ); - transaction_accounts - .get_mut(1) - .unwrap() - .1 - .set_owner(Pubkey::new_unique()); - process_instruction( - transaction_accounts, - instruction_accounts, - Err(InstructionError::IncorrectProgramId), - ); - // Case: Program account not writable let (transaction_accounts, mut instruction_accounts) = get_accounts( &buffer_address, @@ -2686,26 +2663,6 @@ mod tests { Err(InstructionError::InvalidArgument), ); - // Case: Program account not initialized - let (mut transaction_accounts, instruction_accounts) = get_accounts( - &buffer_address, - &upgrade_authority_address, - &upgrade_authority_address, - &elf_orig, - &elf_new, - ); - transaction_accounts - .get_mut(1) - .unwrap() - .1 - .set_state(&UpgradeableLoaderState::Uninitialized) - .unwrap(); - process_instruction( - transaction_accounts, - instruction_accounts, - Err(InstructionError::InvalidAccountData), - ); - // Case: Program ProgramData account mismatch let (mut transaction_accounts, mut instruction_accounts) = get_accounts( &buffer_address, diff --git a/runtime/src/accounts/mod.rs b/runtime/src/accounts/mod.rs index 01df5c9bc01390..1405311a2317f7 100644 --- a/runtime/src/accounts/mod.rs +++ b/runtime/src/accounts/mod.rs @@ -24,7 +24,10 @@ use { loaded_programs::LoadedProgramsForTxBatch, }, solana_sdk::{ - account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, + account::{ + is_builtin, is_executable, mock_executable_meta, Account, AccountSharedData, + ReadableAccount, WritableAccount, + }, account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, feature_set::{ @@ -149,6 +152,22 @@ fn load_transaction_accounts( loaded_programs: &LoadedProgramsForTxBatch, should_collect_rent: bool, ) -> Result { + macro_rules! log_exec_diff { + ($key: expr, $account: expr, $name: expr) => { + let exec2 = is_builtin(&$account) || is_executable(&$account); + if exec2 != $account.executable() { + log::error!( + "exec_flag diffs {} {} {:?} {} {}", + $name, + $key, + &$account, + exec2, + &$account.executable() + ); + } + }; + } + let in_reward_interval = reward_interval == RewardInterval::InsideInterval; // NOTE: this check will never fail because `tx` is sanitized @@ -288,7 +307,9 @@ fn load_transaction_accounts( return Err(TransactionError::InvalidWritableAccount); } - if account.executable() { + log_exec_diff!(key, account, "bpf_upgradable_program"); + + if is_builtin(&account) || is_executable(&account) { // The upgradeable loader requires the derived ProgramData account if let Ok(UpgradeableLoaderState::Program { programdata_address, @@ -306,9 +327,14 @@ fn load_transaction_accounts( return Err(TransactionError::InvalidProgramForExecution); } } - } else if account.executable() && message.is_writable(i) { - error_counters.invalid_writable_account += 1; - return Err(TransactionError::InvalidWritableAccount); + } else { + log_exec_diff!(key, account, "bpf_program"); + if (is_builtin(&account) || is_executable(&account)) + && message.is_writable(i) + { + error_counters.invalid_writable_account += 1; + return Err(TransactionError::InvalidWritableAccount); + } } } @@ -355,15 +381,19 @@ fn load_transaction_accounts( let (program_id, program_account) = accounts .get(program_index) .ok_or(TransactionError::ProgramAccountNotFound)?; - let account_found = accounts_found.get(program_index).unwrap_or(&true); if native_loader::check_id(program_id) { return Ok(account_indices); } + + let account_found = accounts_found.get(program_index).unwrap_or(&true); if !account_found { error_counters.account_not_found += 1; return Err(TransactionError::ProgramAccountNotFound); } - if !program_account.executable() { + + log_exec_diff!(program_id, program_account, "buildin"); + + if !(is_builtin(program_account) || is_executable(program_account)) { error_counters.invalid_program_for_execution += 1; return Err(TransactionError::InvalidProgramForExecution); } @@ -384,8 +414,10 @@ fn load_transaction_accounts( if let Some((owner_account, _)) = accounts_db.load_with_fixed_root(ancestors, owner_id) { + log_exec_diff!(owner_id, owner_account, "buildin"); + if !native_loader::check_id(owner_account.owner()) - || !owner_account.executable() + || !(is_builtin(&owner_account) || is_executable(&owner_account)) { error_counters.invalid_program_for_execution += 1; return Err(TransactionError::InvalidProgramForExecution); @@ -460,6 +492,7 @@ fn account_shared_data_from_program( .ok_or(TransactionError::AccountNotFound)?; program_account.set_owner(**program_owner); program_account.set_executable(true); + program_account.set_data_from_slice(&mock_executable_meta(program_owner)); Ok(program_account) } @@ -981,7 +1014,7 @@ mod tests { let account = AccountSharedData::new(1, 0, &Pubkey::default()); accounts.push((key0, account)); - let account = AccountSharedData::new(40, 1, &native_loader::id()); + let account = AccountSharedData::new(40, 0, &native_loader::id()); accounts.push((key1, account)); let instructions = vec![CompiledInstruction::new(1, &(), vec![0])]; diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index bc4344fdc42301..743a188fd2cb8f 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -123,9 +123,9 @@ use { }, solana_sdk::{ account::{ - create_account_shared_data_with_fields as create_account, from_account, Account, - AccountSharedData, InheritableAccountFields, ReadableAccount, WritableAccount, - PROGRAM_OWNERS, + create_account_shared_data_with_fields as create_account, from_account, + mock_executable_meta, Account, AccountSharedData, InheritableAccountFields, + ReadableAccount, WritableAccount, PROGRAM_OWNERS, }, account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, @@ -3988,7 +3988,6 @@ impl Bank { fn add_precompiled_account_with_owner(&self, program_id: &Pubkey, owner: Pubkey) { if let Some(account) = self.get_account_with_fixed_root(program_id) { if account.executable() { - // The account is already executable, that's all we need return; } else { // malicious account is pre-occupying at program_id @@ -4004,10 +4003,13 @@ impl Bank { // Add a bogus executable account, which will be loaded and ignored. let (lamports, rent_epoch) = self.inherit_specially_retained_account_fields(&None); + + // The account is already executable, that's all we need + let account_data = mock_executable_meta(&owner); let account = AccountSharedData::from(Account { lamports, owner, - data: vec![], + data: account_data, executable: true, rent_epoch, }); diff --git a/sdk/src/account.rs b/sdk/src/account.rs index d94d7dc2c6b6e5..9bffdcf731f209 100644 --- a/sdk/src/account.rs +++ b/sdk/src/account.rs @@ -7,7 +7,7 @@ use { bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable, clock::{Epoch, INITIAL_RENT_EPOCH}, lamports::LamportsError, - loader_v4, + loader_v4, native_loader, pubkey::Pubkey, }, serde::{ @@ -764,6 +764,62 @@ pub const PROGRAM_OWNERS: &[Pubkey] = &[ loader_v4::id(), ]; +pub fn mock_executable_meta(owner: &Pubkey) -> Vec { + if bpf_loader_upgradeable::check_id(owner) { + // For upgradable program account, only + // `UpgradeableLoaderState::Program` variant (i.e. discriminant = 2) is + // *executable*. + return vec![2]; + } + + if loader_v4::check_id(owner) { + // LoaderV4Status (byte_offset = 40) + let mut v = vec![0; 41]; + v[40] = 1; + return v; + } + + vec![1] +} + +/// Return true if the account program is executable. +pub fn is_executable(account: &AccountSharedData) -> bool { + // empty account is not executable. + if account.data().is_empty() { + return false; + } + + // bpf_loader still relies on `executable` on the program account. When the + // program account is finalized, the loader will mark `executable` flag on + // the account. Once we disable the deployment of these two loaders, we can + // remove the following dependency on `executable`. + if bpf_loader::check_id(account.owner()) || bpf_loader_deprecated::check_id(account.owner()) { + return account.executable(); + } + + if bpf_loader_upgradeable::check_id(account.owner()) { + // For upgradable program account, only + // `UpgradeableLoaderState::Program` variant (i.e. discriminant = 2) is + // *executable*. + return account.data()[0] == 2; + } + + if loader_v4::check_id(account.owner()) { + // LoaderV4Status (byte_offset = 40) + // return account.data()[40] != 0; + return false; // TODO: return false for now + } + + false +} + +/// Return true if the account program is a builtin program. Note that for +/// builtin program, even when its account data is empty, it is still be +/// executable, such as vote program etc. +pub fn is_builtin(account: &AccountSharedData) -> bool { + native_loader::check_id(account.owner()) && !account.data().is_empty() +} + #[cfg(test)] pub mod tests { use super::*; diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 77cbb831fb0561..2493f6a7e58094 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -17,7 +17,7 @@ use { }; use { crate::{ - account::{AccountSharedData, ReadableAccount}, + account::{is_builtin, is_executable, AccountSharedData, ReadableAccount}, instruction::InstructionError, pubkey::Pubkey, }, @@ -999,7 +999,14 @@ impl<'a> BorrowedAccount<'a> { /// Returns whether this account is executable (transaction wide) #[inline] pub fn is_executable(&self) -> bool { - self.account.executable() + let executable = is_builtin(&self.account) || is_executable(&self.account); + if executable != self.account.executable() { + log::error!( + "exec_flag diffs borrowed_account {:?}", + (self, executable, self.account.executable()) + ); + } + is_builtin(&self.account) || is_executable(&self.account) } /// Configures whether this account is executable (transaction wide) @@ -1022,11 +1029,11 @@ impl<'a> BorrowedAccount<'a> { return Err(InstructionError::ExecutableModified); } // one can not clear the executable flag - if self.is_executable() && !is_executable { + if self.account.executable() && !is_executable { return Err(InstructionError::ExecutableModified); } // don't touch the account if the executable flag does not change - if self.is_executable() == is_executable { + if self.account.executable() == is_executable { return Ok(()); } self.touch()?;