diff --git a/program-runtime/src/instruction_processor.rs b/program-runtime/src/instruction_processor.rs index 5d26d41dc4d911..347e648f04048a 100644 --- a/program-runtime/src/instruction_processor.rs +++ b/program-runtime/src/instruction_processor.rs @@ -485,7 +485,7 @@ impl InstructionProcessor { ); return Err(InstructionError::AccountNotExecutable); } - let mut program_indices = vec![]; + let mut program_indices = vec![program_account_index]; if program_account.borrow().owner() == &bpf_loader_upgradeable::id() { if let UpgradeableLoaderState::Program { programdata_address, @@ -512,7 +512,6 @@ impl InstructionProcessor { return Err(InstructionError::MissingAccount); } } - program_indices.push(program_account_index); Ok((message, caller_write_privileges, program_indices)) } @@ -594,6 +593,8 @@ impl InstructionProcessor { .get(0) .ok_or(InstructionError::GenericError)?; + let program_id = instruction.program_id(&message.account_keys); + // Verify the calling program hasn't misbehaved invoke_context.verify_and_update(instruction, account_indices, caller_write_privileges)?; @@ -601,7 +602,13 @@ impl InstructionProcessor { invoke_context.set_return_data(Vec::new())?; // Invoke callee - invoke_context.push(message, instruction, program_indices, Some(account_indices))?; + invoke_context.push( + program_id, + message, + instruction, + program_indices, + Some(account_indices), + )?; let mut instruction_processor = InstructionProcessor::default(); for (program_id, process_instruction) in invoke_context.get_programs().iter() { diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 872ec4399a56dd..a1bd3d7b0d6c07 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -199,71 +199,72 @@ fn process_instruction_common( use_jit: bool, ) -> Result<(), InstructionError> { let logger = invoke_context.get_logger(); - let program_id = invoke_context.get_caller()?; - let keyed_accounts = invoke_context.get_keyed_accounts()?; - let first_account = keyed_account_at_index(keyed_accounts, first_instruction_account)?; - let second_account = keyed_account_at_index(keyed_accounts, first_instruction_account + 1); - let (program, next_first_instruction_account) = if first_account.unsigned_key() == program_id { - (first_account, first_instruction_account) - } else if second_account - .as_ref() - .map(|keyed_account| keyed_account.unsigned_key() == program_id) - .unwrap_or(false) - { - (second_account?, first_instruction_account + 1) - } else { - if first_account.executable()? { - ic_logger_msg!(logger, "BPF loader is executable"); - return Err(InstructionError::IncorrectProgramId); - } - (first_account, first_instruction_account) - }; - if program.executable()? { + let first_account = keyed_account_at_index(keyed_accounts, first_instruction_account)?; + if first_account.executable()? { debug_assert_eq!( first_instruction_account, 1 - (invoke_context.invoke_depth() > 1) as usize, ); - if !check_loader_id(&program.owner()?) { - ic_logger_msg!(logger, "Executable account not owned by the BPF loader"); + let program_id = invoke_context.get_caller()?; + if first_account.unsigned_key() != program_id { + ic_logger_msg!(logger, "Program id mismatch"); return Err(InstructionError::IncorrectProgramId); } - let program_data_offset = if bpf_loader_upgradeable::check_id(&program.owner()?) { - if let UpgradeableLoaderState::Program { - programdata_address, - } = program.state()? - { - if programdata_address != *first_account.unsigned_key() { - ic_logger_msg!(logger, "Wrong ProgramData account for this Program account"); - return Err(InstructionError::InvalidArgument); - } - if !matches!( - first_account.state()?, - UpgradeableLoaderState::ProgramData { - slot: _, - upgrade_authority_address: _, + let (first_instruction_account, program_data_offset) = + if bpf_loader_upgradeable::check_id(&first_account.owner()?) { + if let UpgradeableLoaderState::Program { + programdata_address, + } = first_account.state()? + { + let programdata = + keyed_account_at_index(keyed_accounts, first_instruction_account + 1)?; + if programdata_address != *programdata.unsigned_key() { + ic_logger_msg!( + logger, + "Wrong ProgramData account for this Program account" + ); + return Err(InstructionError::InvalidArgument); + } + if !matches!( + programdata.state()?, + UpgradeableLoaderState::ProgramData { + slot: _, + upgrade_authority_address: _, + } + ) { + ic_logger_msg!(logger, "Program has been closed"); + return Err(InstructionError::InvalidAccountData); } - ) { - ic_logger_msg!(logger, "Program has been closed"); + ( + first_instruction_account + 1, + UpgradeableLoaderState::programdata_data_offset()?, + ) + } else { + ic_logger_msg!(logger, "Invalid Program account"); return Err(InstructionError::InvalidAccountData); } - UpgradeableLoaderState::programdata_data_offset()? } else { - ic_logger_msg!(logger, "Invalid Program account"); - return Err(InstructionError::InvalidAccountData); - } - } else { - 0 - }; + (first_instruction_account, 0) + }; + + let keyed_accounts = invoke_context.get_keyed_accounts()?; + let program = keyed_account_at_index(keyed_accounts, first_instruction_account)?; + let loader_id = &program.owner()?; + + if !check_loader_id(loader_id) { + ic_logger_msg!(logger, "Executable account not owned by the BPF loader"); + return Err(InstructionError::IncorrectProgramId); + } let executor = match invoke_context.get_executor(program_id) { Some(executor) => executor, None => { let executor = create_executor( - next_first_instruction_account, + first_instruction_account, program_data_offset, invoke_context, use_jit, @@ -274,32 +275,37 @@ fn process_instruction_common( } }; executor.execute( - next_first_instruction_account, + first_instruction_account, instruction_data, invoke_context, use_jit, - ) + )? } else { debug_assert_eq!(first_instruction_account, 1); + + let program_id = invoke_context.get_caller()?; + if !check_loader_id(program_id) { + ic_logger_msg!(logger, "Invalid BPF loader id"); + return Err(InstructionError::IncorrectProgramId); + } + if bpf_loader_upgradeable::check_id(program_id) { process_loader_upgradeable_instruction( first_instruction_account, instruction_data, invoke_context, use_jit, - ) - } else if bpf_loader::check_id(program_id) || bpf_loader_deprecated::check_id(program_id) { + )?; + } else { process_loader_instruction( first_instruction_account, instruction_data, invoke_context, use_jit, - ) - } else { - ic_logger_msg!(logger, "Invalid BPF loader id"); - Err(InstructionError::IncorrectProgramId) + )?; } } + Ok(()) } fn process_loader_upgradeable_instruction( @@ -3368,8 +3374,8 @@ mod tests { // Try to invoke closed account let keyed_accounts: Vec = vec![ - (false, false, &programdata_address, &programdata_account), (false, false, &program_address, &program_account), + (false, false, &programdata_address, &programdata_account), ]; assert_eq!( Err(InstructionError::InvalidAccountData), diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index b2e3cd1a98dd56..598b92c8548691 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -429,7 +429,7 @@ impl Accounts { // Add loader to chain let program_owner = *program.owner(); - account_indices.insert(0, program_account_index); + if bpf_loader_upgradeable::check_id(&program_owner) { // The upgradeable loader requires the derived ProgramData account if let Ok(UpgradeableLoaderState::Program { @@ -457,6 +457,7 @@ impl Accounts { } } + account_indices.insert(0, program_account_index); program_id = program_owner; } Ok(account_indices) @@ -1923,8 +1924,8 @@ mod tests { let result = loaded_accounts[0].0.as_ref().unwrap(); assert_eq!(result.accounts[..2], accounts[..2]); assert_eq!(result.accounts[result.program_indices[0][0]], accounts[5]); - assert_eq!(result.accounts[result.program_indices[0][1]], accounts[4]); - assert_eq!(result.accounts[result.program_indices[0][2]], accounts[3]); + assert_eq!(result.accounts[result.program_indices[0][1]], accounts[3]); + assert_eq!(result.accounts[result.program_indices[0][2]], accounts[4]); } #[test] diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index e5f28f51a0ab6f..29ebcb00b4c1d3 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -116,6 +116,7 @@ impl<'a> ThisInvokeContext<'a> { impl<'a> InvokeContext for ThisInvokeContext<'a> { fn push( &mut self, + key: &Pubkey, message: &Message, instruction: &CompiledInstruction, program_indices: &[usize], @@ -125,23 +126,6 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { return Err(InstructionError::CallDepth); } - if let Some(index_of_program_id) = program_indices.last() { - let program_id = &self.accounts[*index_of_program_id].0; - let contains = self - .invoke_stack - .iter() - .any(|frame| frame.program_id() == Some(program_id)); - let is_last = if let Some(last_frame) = self.invoke_stack.last() { - last_frame.program_id() == Some(program_id) - } else { - false - }; - if contains && !is_last { - // Reentrancy not allowed unless caller is calling itself - return Err(InstructionError::ReentrancyNotAllowed); - } - } - if self.invoke_stack.is_empty() { self.pre_accounts = Vec::with_capacity(instruction.accounts.len()); let mut work = |_unique_index: usize, account_index: usize| { @@ -156,6 +140,17 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { instruction.visit_each_account(&mut work)?; } + let contains = self.invoke_stack.iter().any(|frame| frame.key == *key); + let is_last = if let Some(last_frame) = self.invoke_stack.last() { + last_frame.key == *key + } else { + false + }; + if contains && !is_last { + // Reentrancy not allowed unless caller is calling itself + return Err(InstructionError::ReentrancyNotAllowed); + } + // Create the KeyedAccounts that will be passed to the program let demote_program_write_locks = self .feature_set @@ -185,9 +180,8 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { ) })) .collect::>(); - self.invoke_stack.push(InvokeContextStackFrame::new( - program_indices.len(), + *key, create_keyed_accounts_unified(keyed_accounts.as_slice()), )); Ok(()) @@ -265,11 +259,11 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { write_privileges: &[bool], ) -> Result<(), InstructionError> { let do_support_realloc = self.feature_set.is_active(&do_support_realloc::id()); - let program_id = self + let stack_frame = self .invoke_stack .last() - .and_then(|frame| frame.program_id()) .ok_or(InstructionError::CallDepth)?; + let program_id = &stack_frame.key; let rent = &self.rent; let logger = &self.logger; let accounts = &self.accounts; @@ -331,7 +325,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { fn get_caller(&self) -> Result<&Pubkey, InstructionError> { self.invoke_stack .last() - .and_then(|frame| frame.program_id()) + .map(|frame| &frame.key) .ok_or(InstructionError::CallDepth) } fn remove_first_keyed_account(&mut self) -> Result<(), InstructionError> { @@ -555,7 +549,7 @@ impl MessageProcessor { invoke_context.set_instruction_index(instruction_index); let result = invoke_context - .push(message, instruction, program_indices, None) + .push(program_id, message, instruction, program_indices, None) .and_then(|_| { instruction_processor .process_instruction(&instruction.data, &mut invoke_context)?; @@ -699,9 +693,10 @@ mod tests { // Check call depth increases and has a limit let mut depth_reached = 0; - for _ in 0..invoke_stack.len() { + for program_id in invoke_stack.iter() { if Err(InstructionError::CallDepth) == invoke_context.push( + program_id, &message, &message.instructions[0], &[MAX_DEPTH + depth_reached], @@ -801,7 +796,13 @@ mod tests { &fee_calculator, ); invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push( + &accounts[0].0, + &message, + &message.instructions[0], + &[0], + None, + ) .unwrap(); assert!(invoke_context .verify(&message, &message.instructions[0], &[0]) @@ -1252,7 +1253,13 @@ mod tests { &fee_calculator, ); invoke_context - .push(&message, &caller_instruction, &program_indices[..1], None) + .push( + &caller_program_id, + &message, + &caller_instruction, + &program_indices[..1], + None, + ) .unwrap(); // not owned account modified by the caller (before the invoke) @@ -1310,7 +1317,13 @@ mod tests { Instruction::new_with_bincode(callee_program_id, &case.0, metas.clone()); let message = Message::new(&[callee_instruction], None); invoke_context - .push(&message, &caller_instruction, &program_indices[..1], None) + .push( + &caller_program_id, + &message, + &caller_instruction, + &program_indices[..1], + None, + ) .unwrap(); let caller_write_privileges = message .account_keys @@ -1397,7 +1410,13 @@ mod tests { &fee_calculator, ); invoke_context - .push(&message, &caller_instruction, &program_indices, None) + .push( + &caller_program_id, + &message, + &caller_instruction, + &program_indices, + None, + ) .unwrap(); // not owned account modified by the invoker @@ -1450,7 +1469,13 @@ mod tests { Instruction::new_with_bincode(callee_program_id, &case.0, metas.clone()); let message = Message::new(&[callee_instruction.clone()], None); invoke_context - .push(&message, &caller_instruction, &program_indices, None) + .push( + &caller_program_id, + &message, + &caller_instruction, + &program_indices, + None, + ) .unwrap(); assert_eq!( InstructionProcessor::native_invoke( diff --git a/sdk/src/process_instruction.rs b/sdk/src/process_instruction.rs index 09213ea5705915..56dc38d2aa300a 100644 --- a/sdk/src/process_instruction.rs +++ b/sdk/src/process_instruction.rs @@ -31,29 +31,23 @@ pub type ProcessInstructionWithContext = fn(usize, &[u8], &mut dyn InvokeContext) -> Result<(), InstructionError>; pub struct InvokeContextStackFrame<'a> { - pub number_of_program_accounts: usize, + pub key: Pubkey, pub keyed_accounts: Vec>, pub keyed_accounts_range: std::ops::Range, } impl<'a> InvokeContextStackFrame<'a> { - pub fn new(number_of_program_accounts: usize, keyed_accounts: Vec>) -> Self { + pub fn new(key: Pubkey, keyed_accounts: Vec>) -> Self { let keyed_accounts_range = std::ops::Range { start: 0, end: keyed_accounts.len(), }; Self { - number_of_program_accounts, + key, keyed_accounts, keyed_accounts_range, } } - - pub fn program_id(&self) -> Option<&Pubkey> { - self.keyed_accounts - .get(self.number_of_program_accounts.saturating_sub(1)) - .map(|keyed_account| keyed_account.unsigned_key()) - } } /// Invocation context passed to loaders @@ -61,6 +55,7 @@ pub trait InvokeContext { /// Push a stack frame onto the invocation stack fn push( &mut self, + key: &Pubkey, message: &Message, instruction: &CompiledInstruction, program_indices: &[usize], @@ -478,17 +473,9 @@ impl<'a> MockInvokeContext<'a> { fee_calculator: FeeCalculator::default(), return_data: (Pubkey::default(), Vec::new()), }; - let number_of_program_accounts = keyed_accounts - .iter() - .position(|keyed_account| keyed_account.unsigned_key() == program_id) - .unwrap_or(0) - .saturating_add(1); invoke_context .invoke_stack - .push(InvokeContextStackFrame::new( - number_of_program_accounts, - keyed_accounts, - )); + .push(InvokeContextStackFrame::new(*program_id, keyed_accounts)); invoke_context } } @@ -511,13 +498,14 @@ pub fn mock_set_sysvar( impl<'a> InvokeContext for MockInvokeContext<'a> { fn push( &mut self, + _key: &Pubkey, _message: &Message, _instruction: &CompiledInstruction, _program_indices: &[usize], _account_indices: Option<&[usize]>, ) -> Result<(), InstructionError> { self.invoke_stack.push(InvokeContextStackFrame::new( - 0, + *_key, create_keyed_accounts_unified(&[]), )); Ok(()) @@ -547,7 +535,7 @@ impl<'a> InvokeContext for MockInvokeContext<'a> { fn get_caller(&self) -> Result<&Pubkey, InstructionError> { self.invoke_stack .last() - .and_then(|frame| frame.program_id()) + .map(|frame| &frame.key) .ok_or(InstructionError::CallDepth) } fn remove_first_keyed_account(&mut self) -> Result<(), InstructionError> {