From f8e9af5f1efdab407679d133baaaa9bf13e8b1bd Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Wed, 20 Jul 2022 14:12:43 +0200 Subject: [PATCH] Cap the number of accounts passed to a bpf program ix (#26630) * Cap the number of accounts passed to a bpf program ix * update bank abi hash * fix ci failures --- programs/bpf/benches/bpf_loader.rs | 2 + programs/bpf/tests/programs.rs | 1 + programs/bpf_loader/src/lib.rs | 16 +- programs/bpf_loader/src/serialization.rs | 189 +++++++++++++++++- rbpf-cli/src/main.rs | 1 + runtime/src/bank.rs | 2 +- sdk/program/src/entrypoint.rs | 5 +- sdk/program/src/instruction.rs | 6 +- sdk/src/feature_set.rs | 5 + storage-proto/proto/transaction_by_addr.proto | 2 +- storage-proto/src/convert.rs | 6 +- 11 files changed, 213 insertions(+), 22 deletions(-) diff --git a/programs/bpf/benches/bpf_loader.rs b/programs/bpf/benches/bpf_loader.rs index 727afacc1..4334463b9 100644 --- a/programs/bpf/benches/bpf_loader.rs +++ b/programs/bpf/benches/bpf_loader.rs @@ -230,6 +230,7 @@ fn bench_create_vm(bencher: &mut Bencher) { .transaction_context .get_current_instruction_context() .unwrap(), + true, // should_cap_ix_accounts ) .unwrap(); @@ -277,6 +278,7 @@ fn bench_instruction_count_tuner(_bencher: &mut Bencher) { .transaction_context .get_current_instruction_context() .unwrap(), + true, // should_cap_ix_accounts ) .unwrap(); diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index aba2e85ae..cf5760427 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -213,6 +213,7 @@ fn run_program(name: &str) -> u64 { .transaction_context .get_current_instruction_context() .unwrap(), + true, // should_cap_ix_accounts ) .unwrap(); diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index fdc87d57f..741f5ea55 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -41,9 +41,10 @@ use { bpf_loader_upgradeable::{self, UpgradeableLoaderState}, entrypoint::{HEAP_LENGTH, SUCCESS}, feature_set::{ - cap_accounts_data_len, disable_bpf_deprecated_load_instructions, - disable_bpf_unresolved_symbols_at_runtime, disable_deploy_of_alloc_free_syscall, - disable_deprecated_loader, enable_bpf_loader_extend_program_data_ix, + cap_accounts_data_len, cap_bpf_program_instruction_accounts, + disable_bpf_deprecated_load_instructions, disable_bpf_unresolved_symbols_at_runtime, + disable_deploy_of_alloc_free_syscall, disable_deprecated_loader, + enable_bpf_loader_extend_program_data_ix, error_on_syscall_bpf_function_hash_collisions, reject_callx_r10, }, instruction::{AccountMeta, InstructionError}, @@ -1285,8 +1286,13 @@ impl Executor for BpfExecutor { let program_id = *instruction_context.get_last_program_key(transaction_context)?; let mut serialize_time = Measure::start("serialize"); - let (mut parameter_bytes, account_lengths) = - serialize_parameters(invoke_context.transaction_context, instruction_context)?; + let (mut parameter_bytes, account_lengths) = serialize_parameters( + invoke_context.transaction_context, + instruction_context, + invoke_context + .feature_set + .is_active(&cap_bpf_program_instruction_accounts::ID), + )?; serialize_time.stop(); let mut create_vm_time = Measure::start("create_vm"); diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 2aa5acd57..0508746b2 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -5,7 +5,7 @@ use { solana_rbpf::{aligned_memory::AlignedMemory, ebpf::HOST_ALIGN}, solana_sdk::{ bpf_loader_deprecated, - entrypoint::{BPF_ALIGN_OF_U128, MAX_PERMITTED_DATA_INCREASE}, + entrypoint::{BPF_ALIGN_OF_U128, MAX_PERMITTED_DATA_INCREASE, NON_DUP_MARKER}, instruction::InstructionError, pubkey::Pubkey, system_instruction::MAX_PERMITTED_DATA_LENGTH, @@ -14,10 +14,20 @@ use { std::{io::prelude::*, mem::size_of}, }; +/// Maximum number of instruction accounts that can be serialized into the +/// BPF VM. +const MAX_INSTRUCTION_ACCOUNTS: u8 = NON_DUP_MARKER; + pub fn serialize_parameters( transaction_context: &TransactionContext, instruction_context: &InstructionContext, + should_cap_ix_accounts: bool, ) -> Result<(AlignedMemory, Vec), InstructionError> { + let num_ix_accounts = instruction_context.get_number_of_instruction_accounts(); + if should_cap_ix_accounts && num_ix_accounts > usize::from(MAX_INSTRUCTION_ACCOUNTS) { + return Err(InstructionError::MaxAccountsExceeded); + } + let is_loader_deprecated = *instruction_context .try_borrow_last_program_account(transaction_context)? .get_owner() @@ -109,7 +119,7 @@ pub fn serialize_parameters_unaligned( } else { let borrowed_account = instruction_context .try_borrow_instruction_account(transaction_context, instruction_account_index)?; - v.write_u8(std::u8::MAX) + v.write_u8(NON_DUP_MARKER) .map_err(|_| InstructionError::InvalidArgument)?; v.write_u8(borrowed_account.is_signer() as u8) .map_err(|_| InstructionError::InvalidArgument)?; @@ -245,7 +255,7 @@ pub fn serialize_parameters_aligned( } else { let borrowed_account = instruction_context .try_borrow_instruction_account(transaction_context, instruction_account_index)?; - v.write_u8(std::u8::MAX) + v.write_u8(NON_DUP_MARKER) .map_err(|_| InstructionError::InvalidArgument)?; v.write_u8(borrowed_account.is_signer() as u8) .map_err(|_| InstructionError::InvalidArgument)?; @@ -381,6 +391,161 @@ mod tests { }, }; + #[test] + fn test_serialize_parameters_with_many_accounts() { + struct TestCase { + num_ix_accounts: usize, + append_dup_account: bool, + should_cap_ix_accounts: bool, + expected_err: Option, + name: &'static str, + } + + for TestCase { + num_ix_accounts, + append_dup_account, + should_cap_ix_accounts, + expected_err, + name, + } in [ + TestCase { + name: "serialize max accounts without cap", + num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), + should_cap_ix_accounts: false, + append_dup_account: false, + expected_err: None, + }, + TestCase { + name: "serialize max accounts and append dup without cap", + num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), + should_cap_ix_accounts: false, + append_dup_account: true, + expected_err: None, + }, + TestCase { + name: "serialize max accounts with cap", + num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), + should_cap_ix_accounts: true, + append_dup_account: false, + expected_err: None, + }, + TestCase { + name: "serialize too many accounts with cap", + num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS) + 1, + should_cap_ix_accounts: true, + append_dup_account: false, + expected_err: Some(InstructionError::MaxAccountsExceeded), + }, + TestCase { + name: "serialize too many accounts and append dup with cap", + num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS), + should_cap_ix_accounts: true, + append_dup_account: true, + expected_err: Some(InstructionError::MaxAccountsExceeded), + }, + // This test case breaks parameter deserialization and can be cleaned up + // when should_cap_ix_accounts is enabled. + // + // TestCase { + // name: "serialize too many accounts and append dup without cap", + // num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS) + 1, + // should_cap_ix_accounts: false, + // append_dup_account: true, + // expected_err: None, + // }, + ] { + let program_id = solana_sdk::pubkey::new_rand(); + let mut transaction_accounts = vec![( + program_id, + AccountSharedData::from(Account { + lamports: 0, + data: vec![], + owner: bpf_loader::id(), + executable: true, + rent_epoch: 0, + }), + )]; + + let instruction_account_keys: Vec = + (0..num_ix_accounts).map(|_| Pubkey::new_unique()).collect(); + + for key in &instruction_account_keys { + transaction_accounts.push(( + *key, + AccountSharedData::from(Account { + lamports: 0, + data: vec![], + owner: program_id, + executable: false, + rent_epoch: 0, + }), + )); + } + + let mut instruction_account_metas: Vec<_> = instruction_account_keys + .iter() + .map(|key| AccountMeta::new_readonly(*key, false)) + .collect(); + if append_dup_account { + instruction_account_metas.push(instruction_account_metas.last().cloned().unwrap()); + } + + let program_indices = [0]; + let instruction_accounts = prepare_mock_invoke_context( + transaction_accounts.clone(), + instruction_account_metas, + &program_indices, + ) + .instruction_accounts; + + let transaction_context = + TransactionContext::new(transaction_accounts, Some(Rent::default()), 1, 1); + let instruction_data = vec![]; + let instruction_context = InstructionContext::new( + 0, + 0, + &program_indices, + &instruction_accounts, + &instruction_data, + ); + + let serialization_result = serialize_parameters( + &transaction_context, + &instruction_context, + should_cap_ix_accounts, + ); + assert_eq!( + serialization_result.as_ref().err(), + expected_err.as_ref(), + "{} test case failed", + name + ); + if expected_err.is_some() { + continue; + } + + let (mut serialized, _account_lengths) = serialization_result.unwrap(); + let (de_program_id, de_accounts, de_instruction_data) = + unsafe { deserialize(serialized.as_slice_mut().first_mut().unwrap() as *mut u8) }; + assert_eq!(de_program_id, &program_id); + assert_eq!(de_instruction_data, &instruction_data); + for (index, account_info) in de_accounts.into_iter().enumerate() { + let ix_account = &instruction_accounts.get(index).unwrap(); + assert_eq!( + account_info.key, + transaction_context + .get_key_of_account_at_index(ix_account.index_in_transaction) + .unwrap() + ); + assert_eq!(account_info.owner, &program_id); + assert!(!account_info.executable); + assert!(account_info.data_is_empty()); + assert!(!account_info.is_writable); + assert!(!account_info.is_signer); + } + } + } + #[test] fn test_serialize_parameters() { let program_id = solana_sdk::pubkey::new_rand(); @@ -496,8 +661,12 @@ mod tests { // check serialize_parameters_aligned - let (mut serialized, account_lengths) = - serialize_parameters(invoke_context.transaction_context, instruction_context).unwrap(); + let (mut serialized, account_lengths) = serialize_parameters( + invoke_context.transaction_context, + instruction_context, + true, + ) + .unwrap(); let (de_program_id, de_accounts, de_instruction_data) = unsafe { deserialize(serialized.as_slice_mut().first_mut().unwrap() as *mut u8) }; @@ -568,8 +737,12 @@ mod tests { .borrow_mut() .set_owner(bpf_loader_deprecated::id()); - let (mut serialized, account_lengths) = - serialize_parameters(invoke_context.transaction_context, instruction_context).unwrap(); + let (mut serialized, account_lengths) = serialize_parameters( + invoke_context.transaction_context, + instruction_context, + true, + ) + .unwrap(); let (de_program_id, de_accounts, de_instruction_data) = unsafe { deserialize_unaligned(serialized.as_slice_mut().first_mut().unwrap() as *mut u8) @@ -630,7 +803,7 @@ mod tests { for _ in 0..num_accounts { let dup_info = *(input.add(offset) as *const u8); offset += size_of::(); - if dup_info == std::u8::MAX { + if dup_info == NON_DUP_MARKER { #[allow(clippy::cast_ptr_alignment)] let is_signer = *(input.add(offset) as *const u8) != 0; offset += size_of::(); diff --git a/rbpf-cli/src/main.rs b/rbpf-cli/src/main.rs index 1e772ce4e..2b66e4086 100644 --- a/rbpf-cli/src/main.rs +++ b/rbpf-cli/src/main.rs @@ -236,6 +236,7 @@ native machine code before execting it in the virtual machine.", .transaction_context .get_current_instruction_context() .unwrap(), + true, // should_cap_ix_accounts ) .unwrap(); let compute_meter = invoke_context.get_compute_meter(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 560e56797..c0453d806 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -263,7 +263,7 @@ impl RentDebits { } pub type BankStatusCache = StatusCache>; -#[frozen_abi(digest = "2YZk2K45HmmAafmxPJnYVXyQ7uA7WuBrRkpwrCawdK31")] +#[frozen_abi(digest = "HEJXoycXvGT2pwMuKcUKzzbeemnqbfrUC4jHZx1ncaWv")] pub type BankSlotDelta = SlotDelta>; // Eager rent collection repeats in cyclic manner. diff --git a/sdk/program/src/entrypoint.rs b/sdk/program/src/entrypoint.rs index ba9fc4d9d..109ed1f09 100644 --- a/sdk/program/src/entrypoint.rs +++ b/sdk/program/src/entrypoint.rs @@ -36,6 +36,9 @@ pub const HEAP_START_ADDRESS: u64 = 0x300000000; /// Length of the heap memory region used for program heap. pub const HEAP_LENGTH: usize = 32 * 1024; +/// Value used to indicate that a serialized account is not a duplicate +pub const NON_DUP_MARKER: u8 = u8::MAX; + /// Declare the program entry point and set up global handlers. /// /// This macro emits the common boilerplate necessary to begin program @@ -283,7 +286,7 @@ pub unsafe fn deserialize<'a>(input: *mut u8) -> (&'a Pubkey, Vec(); - if dup_info == std::u8::MAX { + if dup_info == NON_DUP_MARKER { #[allow(clippy::cast_ptr_alignment)] let is_signer = *(input.add(offset) as *const u8) != 0; offset += size_of::(); diff --git a/sdk/program/src/instruction.rs b/sdk/program/src/instruction.rs index 49de9dcc8..7b2931847 100644 --- a/sdk/program/src/instruction.rs +++ b/sdk/program/src/instruction.rs @@ -253,9 +253,9 @@ pub enum InstructionError { #[error("Account data allocation exceeded the maximum accounts data size limit")] MaxAccountsDataSizeExceeded, - /// Active vote account close - #[error("Cannot close vote account unless it stopped voting at least one full epoch ago")] - ActiveVoteAccountClose, + /// Max accounts exceeded + #[error("Max accounts exceeded")] + MaxAccountsExceeded, // Note: For any new error added here an equivalent ProgramError and its // conversions must also be added } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 96a90e83f..995ad8916 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -464,6 +464,10 @@ pub mod prevent_crediting_accounts_that_end_rent_paying { solana_sdk::declare_id!("812kqX67odAp5NFwM8D2N24cku7WTm9CHUTFUXaDkWPn"); } +pub mod cap_bpf_program_instruction_accounts { + solana_sdk::declare_id!("9k5ijzTbYPtjzu8wj2ErH9v45xecHzQ1x4PMYMMxFgdM"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -574,6 +578,7 @@ lazy_static! { (enable_bpf_loader_extend_program_data_ix::id(), "enable bpf upgradeable loader ExtendProgramData instruction #25234"), (enable_early_verification_of_account_modifications::id(), "enable early verification of account modifications #25899"), (prevent_crediting_accounts_that_end_rent_paying::id(), "prevent crediting rent paying accounts #26606"), + (cap_bpf_program_instruction_accounts::id(), "enforce max number of accounts per bpf program instruction #26628"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/storage-proto/proto/transaction_by_addr.proto b/storage-proto/proto/transaction_by_addr.proto index 2d582ba52..4c95b1829 100644 --- a/storage-proto/proto/transaction_by_addr.proto +++ b/storage-proto/proto/transaction_by_addr.proto @@ -121,7 +121,7 @@ enum InstructionErrorType { UNSUPPORTED_SYSVAR = 48; ILLEGAL_OWNER = 49; MAX_ACCOUNTS_DATA_SIZE_EXCEEDED = 50; - ACTIVE_VOTE_ACCOUNT_CLOSE = 51; + MAX_ACCOUNTS_EXCEEDED = 51; } message UnixTimestamp { diff --git a/storage-proto/src/convert.rs b/storage-proto/src/convert.rs index 0b32da668..4998d5f9a 100644 --- a/storage-proto/src/convert.rs +++ b/storage-proto/src/convert.rs @@ -705,7 +705,7 @@ impl TryFrom for TransactionError { 48 => InstructionError::UnsupportedSysvar, 49 => InstructionError::IllegalOwner, 50 => InstructionError::MaxAccountsDataSizeExceeded, - 51 => InstructionError::ActiveVoteAccountClose, + 51 => InstructionError::MaxAccountsExceeded, _ => return Err("Invalid InstructionError"), }; @@ -1022,8 +1022,8 @@ impl From for tx_by_addr::TransactionError { InstructionError::MaxAccountsDataSizeExceeded => { tx_by_addr::InstructionErrorType::MaxAccountsDataSizeExceeded } - InstructionError::ActiveVoteAccountClose => { - tx_by_addr::InstructionErrorType::ActiveVoteAccountClose + InstructionError::MaxAccountsExceeded => { + tx_by_addr::InstructionErrorType::MaxAccountsExceeded } } as i32, custom: match instruction_error {