From 3bf9c2e7e7c080e236060dd8a833771bff6aad7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Mon, 4 Nov 2024 22:13:28 +0100 Subject: [PATCH] Feature - Remove accounts executable flag checks (#2182) * Adds the feature. * Adds the feature gate logic. * Adjusts tests. * Adds deprecation attributes. * Set is_executable flag upon initialization in loader-v4. --- program-runtime/src/invoke_context.rs | 11 +- programs/bpf_loader/src/lib.rs | 138 ++++++++++++++++++----- programs/bpf_loader/src/serialization.rs | 2 + programs/bpf_loader/src/syscalls/cpi.rs | 1 + programs/loader-v4/src/lib.rs | 1 + programs/sbf/tests/programs.rs | 16 +-- runtime/src/bank/tests.rs | 4 +- sdk/feature-set/src/lib.rs | 5 + sdk/src/transaction_context.rs | 28 ++++- svm/src/account_loader.rs | 18 ++- svm/src/transaction_processor.rs | 8 +- 11 files changed, 183 insertions(+), 49 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index c96da3e1da3539..7cb72f21ee0e5b 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -8,7 +8,9 @@ use { sysvar_cache::SysvarCache, }, solana_compute_budget::compute_budget::ComputeBudget, - solana_feature_set::{move_precompile_verification_to_svm, FeatureSet}, + solana_feature_set::{ + move_precompile_verification_to_svm, remove_accounts_executable_flag_checks, FeatureSet, + }, solana_log_collector::{ic_msg, LogCollector}, solana_measure::measure::Measure, solana_rbpf::{ @@ -435,7 +437,12 @@ impl<'a> InvokeContext<'a> { })?; let borrowed_program_account = instruction_context .try_borrow_instruction_account(self.transaction_context, program_account_index)?; - if !borrowed_program_account.is_executable() { + #[allow(deprecated)] + if !self + .get_feature_set() + .is_active(&remove_accounts_executable_flag_checks::id()) + && !borrowed_program_account.is_executable() + { ic_msg!(self, "Account {} is not executable", callee_program_id); return Err(InstructionError::AccountNotExecutable); } diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 5e81062c504e97..883f0d2548aef0 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -8,6 +8,7 @@ use { solana_compute_budget::compute_budget::MAX_INSTRUCTION_STACK_DEPTH, solana_feature_set::{ bpf_account_data_direct_mapping, enable_bpf_loader_set_authority_checked_ix, + remove_accounts_executable_flag_checks, }, solana_log_collector::{ic_logger_msg, ic_msg, LogCollector}, solana_measure::measure::Measure, @@ -423,14 +424,28 @@ pub fn process_instruction_inner( Err(InstructionError::UnsupportedProgramId) } else { ic_logger_msg!(log_collector, "Invalid BPF loader id"); - Err(InstructionError::IncorrectProgramId) + Err( + if invoke_context + .get_feature_set() + .is_active(&remove_accounts_executable_flag_checks::id()) + { + InstructionError::UnsupportedProgramId + } else { + InstructionError::IncorrectProgramId + }, + ) } .map(|_| 0) .map_err(|error| Box::new(error) as Box); } // Program Invocation - if !program_account.is_executable() { + #[allow(deprecated)] + if !invoke_context + .get_feature_set() + .is_active(&remove_accounts_executable_flag_checks::id()) + && !program_account.is_executable() + { ic_logger_msg!(log_collector, "Program is not executable"); return Err(Box::new(InstructionError::IncorrectProgramId)); } @@ -441,7 +456,14 @@ pub fn process_instruction_inner( .find(program_account.get_key()) .ok_or_else(|| { ic_logger_msg!(log_collector, "Program is not cached"); - InstructionError::InvalidAccountData + if invoke_context + .get_feature_set() + .is_active(&remove_accounts_executable_flag_checks::id()) + { + InstructionError::UnsupportedProgramId + } else { + InstructionError::InvalidAccountData + } })?; drop(program_account); get_or_create_executor_time.stop(); @@ -456,10 +478,28 @@ pub fn process_instruction_inner( | ProgramCacheEntryType::Closed | ProgramCacheEntryType::DelayVisibility => { ic_logger_msg!(log_collector, "Program is not deployed"); - Err(Box::new(InstructionError::InvalidAccountData) as Box) + let instruction_error = if invoke_context + .get_feature_set() + .is_active(&remove_accounts_executable_flag_checks::id()) + { + InstructionError::UnsupportedProgramId + } else { + InstructionError::InvalidAccountData + }; + Err(Box::new(instruction_error) as Box) } ProgramCacheEntryType::Loaded(executable) => execute(executable, invoke_context), - _ => Err(Box::new(InstructionError::IncorrectProgramId) as Box), + _ => { + let instruction_error = if invoke_context + .get_feature_set() + .is_active(&remove_accounts_executable_flag_checks::id()) + { + InstructionError::UnsupportedProgramId + } else { + InstructionError::IncorrectProgramId + }; + Err(Box::new(instruction_error) as Box) + } } .map(|_| 0) } @@ -718,7 +758,12 @@ fn process_loader_upgradeable_instruction( let program = instruction_context.try_borrow_instruction_account(transaction_context, 1)?; - if !program.is_executable() { + #[allow(deprecated)] + if !invoke_context + .get_feature_set() + .is_active(&remove_accounts_executable_flag_checks::id()) + && !program.is_executable() + { ic_logger_msg!(log_collector, "Program account not executable"); return Err(InstructionError::AccountNotExecutable); } @@ -1457,13 +1502,20 @@ pub fn execute<'a, 'b: 'a>( instruction_account_index as IndexOfAccount, )?; - error = EbpfError::SyscallError(Box::new(if account.is_executable() { - InstructionError::ExecutableDataModified - } else if account.is_writable() { - InstructionError::ExternalAccountDataModified - } else { - InstructionError::ReadonlyDataModified - })); + error = EbpfError::SyscallError(Box::new( + #[allow(deprecated)] + if !invoke_context + .get_feature_set() + .is_active(&remove_accounts_executable_flag_checks::id()) + && account.is_executable() + { + InstructionError::ExecutableDataModified + } else if account.is_writable() { + InstructionError::ExternalAccountDataModified + } else { + InstructionError::ReadonlyDataModified + }, + )); } } } @@ -1636,7 +1688,7 @@ mod tests { fn test_bpf_loader_invoke_main() { let loader_id = bpf_loader::id(); let program_id = Pubkey::new_unique(); - let mut program_account = + let program_account = load_program_account_from_elf(&loader_id, "test_elfs/out/noop_aligned.so"); let parameter_id = Pubkey::new_unique(); let parameter_account = AccountSharedData::new(1, 0, &loader_id); @@ -1686,7 +1738,7 @@ mod tests { &[], vec![ (program_id, program_account.clone()), - (parameter_id, parameter_account), + (parameter_id, parameter_account.clone()), ], vec![parameter_meta.clone(), parameter_meta], Ok(()), @@ -1697,7 +1749,7 @@ mod tests { &loader_id, vec![0], &[], - vec![(program_id, program_account.clone())], + vec![(program_id, program_account)], Vec::new(), Err(InstructionError::ProgramFailedToComplete), Entrypoint::vm, @@ -1709,14 +1761,29 @@ mod tests { ); // Case: Account not a program - program_account.set_executable(false); + mock_process_instruction( + &loader_id, + vec![0], + &[], + vec![(program_id, parameter_account.clone())], + Vec::new(), + Err(InstructionError::IncorrectProgramId), + Entrypoint::vm, + |invoke_context| { + let mut feature_set = invoke_context.get_feature_set().clone(); + feature_set.deactivate(&remove_accounts_executable_flag_checks::id()); + invoke_context.mock_set_feature_set(Arc::new(feature_set)); + test_utils::load_all_invoked_programs(invoke_context); + }, + |_invoke_context| {}, + ); process_instruction( &loader_id, &[0], &[], - vec![(program_id, program_account)], + vec![(program_id, parameter_account)], Vec::new(), - Err(InstructionError::IncorrectProgramId), + Err(InstructionError::UnsupportedProgramId), ); } @@ -2379,22 +2446,35 @@ mod tests { ); // Case: Program account not executable - let (mut transaction_accounts, instruction_accounts) = get_accounts( + let (transaction_accounts, mut instruction_accounts) = get_accounts( &buffer_address, &upgrade_authority_address, &upgrade_authority_address, &elf_orig, &elf_new, ); - transaction_accounts - .get_mut(1) - .unwrap() - .1 - .set_executable(false); - process_instruction( - transaction_accounts, - instruction_accounts, + *instruction_accounts.get_mut(1).unwrap() = instruction_accounts.get(2).unwrap().clone(); + let instruction_data = bincode::serialize(&UpgradeableLoaderInstruction::Upgrade).unwrap(); + mock_process_instruction( + &bpf_loader_upgradeable::id(), + Vec::new(), + &instruction_data, + transaction_accounts.clone(), + instruction_accounts.clone(), Err(InstructionError::AccountNotExecutable), + Entrypoint::vm, + |invoke_context| { + let mut feature_set = invoke_context.get_feature_set().clone(); + feature_set.deactivate(&remove_accounts_executable_flag_checks::id()); + invoke_context.mock_set_feature_set(Arc::new(feature_set)); + test_utils::load_all_invoked_programs(invoke_context); + }, + |_invoke_context| {}, + ); + process_instruction( + transaction_accounts.clone(), + instruction_accounts.clone(), + Err(InstructionError::InvalidAccountData), ); // Case: Program account now owned by loader @@ -3593,7 +3673,7 @@ mod tests { (program_address, program_account.clone()), ], Vec::new(), - Err(InstructionError::InvalidAccountData), + Err(InstructionError::UnsupportedProgramId), ); // Case: Reopen should fail diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 2cb2f20b162280..15c5820dc23924 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -339,6 +339,7 @@ fn serialize_parameters_unaligned( s.write::((account.get_data().len() as u64).to_le()); let vm_data_addr = s.write_account(&mut account)?; let vm_owner_addr = s.write_all(account.get_owner().as_ref()); + #[allow(deprecated)] s.write::(account.is_executable() as u8); s.write::((account.get_rent_epoch()).to_le()); accounts_metadata.push(SerializedAccountMetadata { @@ -467,6 +468,7 @@ fn serialize_parameters_aligned( s.write::(NON_DUP_MARKER); s.write::(borrowed_account.is_signer() as u8); s.write::(borrowed_account.is_writable() as u8); + #[allow(deprecated)] s.write::(borrowed_account.is_executable() as u8); s.write_all(&[0u8, 0, 0, 0]); let vm_key_addr = s.write_all(borrowed_account.get_key().as_ref()); diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 1f4373fd4af514..7816cc473b905b 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -890,6 +890,7 @@ where .transaction_context .get_key_of_account_at_index(instruction_account.index_in_transaction)?; + #[allow(deprecated)] if callee_account.is_executable() { // Use the known account consume_compute_meter( diff --git a/programs/loader-v4/src/lib.rs b/programs/loader-v4/src/lib.rs index c35b67a45ff9ce..7f77636bcc60a5 100644 --- a/programs/loader-v4/src/lib.rs +++ b/programs/loader-v4/src/lib.rs @@ -198,6 +198,7 @@ pub fn process_instruction_truncate( LoaderV4State::program_data_offset().saturating_add(new_size as usize), )?; if is_initialization { + program.set_executable(true)?; let state = get_state_mut(program.get_data_mut()?)?; state.slot = 0; state.status = LoaderV4Status::Retracted; diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 84f9dbc96c57a3..991d7a8695444c 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -921,15 +921,15 @@ fn test_program_sbf_invoke_sanity() { do_invoke_failure_test_local( TEST_PPROGRAM_NOT_OWNED_BY_LOADER, - TransactionError::InstructionError(0, InstructionError::AccountNotExecutable), - &[], + TransactionError::InstructionError(0, InstructionError::UnsupportedProgramId), + &[argument_keypair.pubkey()], None, ); do_invoke_failure_test_local( TEST_PPROGRAM_NOT_EXECUTABLE, - TransactionError::InstructionError(0, InstructionError::AccountNotExecutable), - &[], + TransactionError::InstructionError(0, InstructionError::UnsupportedProgramId), + &[unexecutable_program_keypair.pubkey()], None, ); @@ -1898,7 +1898,7 @@ fn test_program_sbf_invoke_in_same_tx_as_deployment() { let (result, _, _) = process_transaction_and_record_inner(&bank, tx); assert_eq!( result.unwrap_err(), - TransactionError::InstructionError(2, InstructionError::InvalidAccountData), + TransactionError::InstructionError(2, InstructionError::UnsupportedProgramId), ); } } @@ -2009,7 +2009,7 @@ fn test_program_sbf_invoke_in_same_tx_as_redeployment() { let (result, _, _) = process_transaction_and_record_inner(&bank, tx); assert_eq!( result.unwrap_err(), - TransactionError::InstructionError(1, InstructionError::InvalidAccountData), + TransactionError::InstructionError(1, InstructionError::UnsupportedProgramId), ); } } @@ -2104,7 +2104,7 @@ fn test_program_sbf_invoke_in_same_tx_as_undeployment() { let (result, _, _) = process_transaction_and_record_inner(&bank, tx); assert_eq!( result.unwrap_err(), - TransactionError::InstructionError(1, InstructionError::InvalidAccountData), + TransactionError::InstructionError(1, InstructionError::UnsupportedProgramId), ); } } @@ -4653,7 +4653,7 @@ fn test_deny_executable_write() { let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); assert_eq!( result.unwrap_err().unwrap(), - TransactionError::InstructionError(0, InstructionError::ExecutableDataModified) + TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified) ); } } diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 57afa678eeb3f0..70e730affa745d 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -7230,7 +7230,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { bank.process_transaction(&transaction), Err(TransactionError::InstructionError( 0, - InstructionError::InvalidAccountData + InstructionError::UnsupportedProgramId )), ); { @@ -7254,7 +7254,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { bank.process_transaction(&transaction), Err(TransactionError::InstructionError( 0, - InstructionError::InvalidAccountData, + InstructionError::UnsupportedProgramId, )), ); { diff --git a/sdk/feature-set/src/lib.rs b/sdk/feature-set/src/lib.rs index 83d59749758067..e27cabe6f3cbcd 100644 --- a/sdk/feature-set/src/lib.rs +++ b/sdk/feature-set/src/lib.rs @@ -869,6 +869,10 @@ pub mod reenable_sbpf_v1_execution { solana_pubkey::declare_id!("TestFeature21111111111111111111111111111111"); } +pub mod remove_accounts_executable_flag_checks { + solana_pubkey::declare_id!("FfgtauHUWKeXTzjXkua9Px4tNGBFHKZ9WaigM5VbbzFx"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -1081,6 +1085,7 @@ lazy_static! { (partitioned_epoch_rewards_superfeature::id(), "replaces enable_partitioned_epoch_reward to enable partitioned rewards at epoch boundary SIMD-0118"), (disable_sbpf_v1_execution::id(), "Disables execution of SBPFv1 programs"), (reenable_sbpf_v1_execution::id(), "Re-enables execution of SBPFv1 programs"), + (remove_accounts_executable_flag_checks::id(), "Remove checks of accounts is_executable flag SIMD-0162"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 95f65c3af55fe0..6cb9513a127537 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -140,6 +140,8 @@ pub struct TransactionContext { return_data: TransactionReturnData, accounts_resize_delta: RefCell, #[cfg(not(target_os = "solana"))] + remove_accounts_executable_flag_checks: bool, + #[cfg(not(target_os = "solana"))] rent: Rent, /// Useful for debugging to filter by or to look it up on the explorer #[cfg(all(not(target_os = "solana"), feature = "full", debug_assertions))] @@ -168,12 +170,18 @@ impl TransactionContext { instruction_trace: vec![InstructionContext::default()], return_data: TransactionReturnData::default(), accounts_resize_delta: RefCell::new(0), + remove_accounts_executable_flag_checks: true, rent, #[cfg(all(not(target_os = "solana"), feature = "full", debug_assertions))] signature: Signature::default(), } } + #[cfg(not(target_os = "solana"))] + pub fn set_remove_accounts_executable_flag_checks(&mut self, enabled: bool) { + self.remove_accounts_executable_flag_checks = enabled; + } + /// Used in mock_process_instruction #[cfg(not(target_os = "solana"))] pub fn deconstruct_without_keys(self) -> Result, InstructionError> { @@ -746,7 +754,7 @@ impl<'a> BorrowedAccount<'a> { return Err(InstructionError::ModifiedProgramId); } // and only if the account is not executable - if self.is_executable() { + if self.is_executable_internal() { return Err(InstructionError::ModifiedProgramId); } // and only if the data is zero-initialized or empty @@ -780,7 +788,7 @@ impl<'a> BorrowedAccount<'a> { return Err(InstructionError::ReadonlyLamportChange); } // The balance of executable accounts may not change - if self.is_executable() { + if self.is_executable_internal() { return Err(InstructionError::ExecutableLamportChange); } // don't touch the account if the lamports do not change @@ -992,10 +1000,21 @@ impl<'a> BorrowedAccount<'a> { /// Returns whether this account is executable (transaction wide) #[inline] + #[deprecated(since = "2.1.0", note = "Use `get_owner` instead")] pub fn is_executable(&self) -> bool { self.account.executable() } + /// Feature gating to remove `is_executable` flag related checks + #[cfg(not(target_os = "solana"))] + #[inline] + fn is_executable_internal(&self) -> bool { + !self + .transaction_context + .remove_accounts_executable_flag_checks + && self.account.executable() + } + /// Configures whether this account is executable (transaction wide) #[cfg(not(target_os = "solana"))] pub fn set_executable(&mut self, is_executable: bool) -> Result<(), InstructionError> { @@ -1016,10 +1035,11 @@ impl<'a> BorrowedAccount<'a> { return Err(InstructionError::ExecutableModified); } // one can not clear the executable flag - if self.is_executable() && !is_executable { + if self.is_executable_internal() && !is_executable { return Err(InstructionError::ExecutableModified); } // don't touch the account if the executable flag does not change + #[allow(deprecated)] if self.is_executable() == is_executable { return Ok(()); } @@ -1073,7 +1093,7 @@ impl<'a> BorrowedAccount<'a> { #[cfg(not(target_os = "solana"))] pub fn can_data_be_changed(&self) -> Result<(), InstructionError> { // Only non-executable accounts data can be changed - if self.is_executable() { + if self.is_executable_internal() { return Err(InstructionError::ExecutableDataModified); } // and only if the account is writable diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 2e47957d7f31dd..99105e0bd9183e 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -356,7 +356,9 @@ fn load_transaction_accounts( return Err(TransactionError::ProgramAccountNotFound); } - if !program_account.executable() { + if !feature_set.is_active(&feature_set::remove_accounts_executable_flag_checks::id()) + && !program_account.executable() + { error_metrics.invalid_program_for_execution += 1; return Err(TransactionError::InvalidProgramForExecution); } @@ -373,7 +375,9 @@ fn load_transaction_accounts( { if let Some(owner_account) = callbacks.get_account_shared_data(owner_id) { if !native_loader::check_id(owner_account.owner()) - || !owner_account.executable() + || (!feature_set + .is_active(&feature_set::remove_accounts_executable_flag_checks::id()) + && !owner_account.executable()) { error_metrics.invalid_program_for_execution += 1; return Err(TransactionError::InvalidProgramForExecution); @@ -882,7 +886,15 @@ mod tests { instructions, ); - let load_results = load_accounts_aux_test(tx, &accounts, &mut error_metrics); + let mut feature_set = FeatureSet::all_enabled(); + feature_set.deactivate(&feature_set::remove_accounts_executable_flag_checks::id()); + let load_results = load_accounts_with_features_and_rent( + tx, + &accounts, + &RentCollector::default(), + &mut error_metrics, + &mut feature_set, + ); assert_eq!(error_metrics.invalid_program_for_execution, 1); assert_eq!(load_results.len(), 1); diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 7b54dfbf878297..5cd0c4f602a4bd 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -25,7 +25,8 @@ use { }, solana_compute_budget::compute_budget::ComputeBudget, solana_feature_set::{ - enable_transaction_loading_failure_fees, remove_rounding_in_fee_calculation, FeatureSet, + enable_transaction_loading_failure_fees, remove_accounts_executable_flag_checks, + remove_rounding_in_fee_calculation, FeatureSet, }, solana_log_collector::LogCollector, solana_measure::{measure::Measure, measure_us}, @@ -780,6 +781,11 @@ impl TransactionBatchProcessor { compute_budget.max_instruction_stack_depth, compute_budget.max_instruction_trace_length, ); + transaction_context.set_remove_accounts_executable_flag_checks( + environment + .feature_set + .is_active(&remove_accounts_executable_flag_checks::id()), + ); #[cfg(debug_assertions)] transaction_context.set_signature(tx.signature());