From cc38ae72e7c98fa69bfeff919254b3e1f6d0a562 Mon Sep 17 00:00:00 2001 From: Jack May Date: Thu, 11 Mar 2021 19:28:21 -0800 Subject: [PATCH] Skip deserialization of readonly accounts (#15813) --- programs/bpf/c/src/invoke/invoke.c | 33 +-- programs/bpf/c/src/invoked/invoked.c | 2 +- programs/bpf/rust/invoke/src/lib.rs | 31 ++- programs/bpf/rust/invoked/src/processor.rs | 5 +- programs/bpf/tests/programs.rs | 17 +- programs/bpf_loader/src/lib.rs | 8 +- programs/bpf_loader/src/serialization.rs | 225 +++++++++++++++++---- sdk/src/feature_set.rs | 5 + 8 files changed, 245 insertions(+), 81 deletions(-) diff --git a/programs/bpf/c/src/invoke/invoke.c b/programs/bpf/c/src/invoke/invoke.c index 8b476bce5690f6..0ae306220e6ef6 100644 --- a/programs/bpf/c/src/invoke/invoke.c +++ b/programs/bpf/c/src/invoke/invoke.c @@ -17,7 +17,6 @@ static const uint8_t TEST_INSTRUCTION_META_TOO_LARGE = 10; static const uint8_t TEST_RETURN_ERROR = 11; static const uint8_t TEST_PRIVILEGE_DEESCALATION_ESCALATION_SIGNER = 12; static const uint8_t TEST_PRIVILEGE_DEESCALATION_ESCALATION_WRITABLE = 13; -static const uint8_t TEST_WRITE_DEESCALATION = 14; static const int MINT_INDEX = 0; static const int ARGUMENT_INDEX = 1; @@ -272,6 +271,24 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_assert(accounts[ARGUMENT_INDEX].data[i] == 0); } } + sol_log("Test writable deescalation"); + { + uint8_t buffer[10]; + for (int i = 0; i < 10; i++) { + buffer[i] = accounts[INVOKED_ARGUMENT_INDEX].data[i]; + } + SolAccountMeta arguments[] = { + {accounts[INVOKED_ARGUMENT_INDEX].key, false, false}}; + uint8_t data[] = {WRITE_ACCOUNT, 10}; + const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, + arguments, SOL_ARRAY_SIZE(arguments), + data, SOL_ARRAY_SIZE(data)}; + sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts)); + + for (int i = 0; i < 10; i++) { + sol_assert(buffer[i] == accounts[INVOKED_ARGUMENT_INDEX].data[i]); + } + } break; } case TEST_PRIVILEGE_ESCALATION_SIGNER: { @@ -474,7 +491,6 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts)); break; } - case TEST_PRIVILEGE_DEESCALATION_ESCALATION_SIGNER: { sol_log("Test privilege deescalation escalation signer"); sol_assert(true == accounts[INVOKED_ARGUMENT_INDEX].is_signer); @@ -505,19 +521,6 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts))); break; } - - case TEST_WRITE_DEESCALATION: { - sol_log("Test writable deescalation"); - - SolAccountMeta arguments[] = { - {accounts[INVOKED_ARGUMENT_INDEX].key, false, false}}; - uint8_t data[] = {WRITE_ACCOUNT, 10}; - const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, - arguments, SOL_ARRAY_SIZE(arguments), - data, SOL_ARRAY_SIZE(data)}; - sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts)); - break; - } default: sol_panic(); } diff --git a/programs/bpf/c/src/invoked/invoked.c b/programs/bpf/c/src/invoked/invoked.c index f554ca9a2a2699..e0b6f7f5515115 100644 --- a/programs/bpf/c/src/invoked/invoked.c +++ b/programs/bpf/c/src/invoked/invoked.c @@ -160,7 +160,7 @@ extern uint64_t entrypoint(const uint8_t *input) { } case VERIFY_PRIVILEGE_ESCALATION: { - sol_log("Should never get here!"); + sol_log("Verify privilege escalation"); break; } diff --git a/programs/bpf/rust/invoke/src/lib.rs b/programs/bpf/rust/invoke/src/lib.rs index e5b119b794f770..330fc1d5fbf0a6 100644 --- a/programs/bpf/rust/invoke/src/lib.rs +++ b/programs/bpf/rust/invoke/src/lib.rs @@ -29,7 +29,6 @@ const TEST_INSTRUCTION_META_TOO_LARGE: u8 = 10; const TEST_RETURN_ERROR: u8 = 11; const TEST_PRIVILEGE_DEESCALATION_ESCALATION_SIGNER: u8 = 12; const TEST_PRIVILEGE_DEESCALATION_ESCALATION_WRITABLE: u8 = 13; -const TEST_WRITE_DEESCALATION: u8 = 14; // const MINT_INDEX: usize = 0; const ARGUMENT_INDEX: usize = 1; @@ -354,6 +353,27 @@ fn process_instruction( assert_eq!(data[i as usize], 42); } } + + msg!("Test writable deescalation"); + { + const NUM_BYTES: usize = 10; + let mut buffer = [0; NUM_BYTES]; + buffer.copy_from_slice( + &accounts[INVOKED_ARGUMENT_INDEX].data.borrow_mut()[..NUM_BYTES], + ); + + let instruction = create_instruction( + *accounts[INVOKED_PROGRAM_INDEX].key, + &[(accounts[INVOKED_ARGUMENT_INDEX].key, false, false)], + vec![WRITE_ACCOUNT, NUM_BYTES as u8], + ); + let _ = invoke(&instruction, accounts); + + assert_eq!( + buffer, + accounts[INVOKED_ARGUMENT_INDEX].data.borrow_mut()[..NUM_BYTES] + ); + } } TEST_PRIVILEGE_ESCALATION_SIGNER => { msg!("Test privilege escalation signer"); @@ -557,15 +577,6 @@ fn process_instruction( ); invoke(&invoked_instruction, accounts)?; } - TEST_WRITE_DEESCALATION => { - msg!("Test writable deescalation"); - let instruction = create_instruction( - *accounts[INVOKED_PROGRAM_INDEX].key, - &[(accounts[INVOKED_ARGUMENT_INDEX].key, false, false)], - vec![WRITE_ACCOUNT, 10], - ); - let _ = invoke(&instruction, accounts); - } _ => panic!(), } diff --git a/programs/bpf/rust/invoked/src/processor.rs b/programs/bpf/rust/invoked/src/processor.rs index 120160fd3afd17..1c9c8c49483daf 100644 --- a/programs/bpf/rust/invoked/src/processor.rs +++ b/programs/bpf/rust/invoked/src/processor.rs @@ -199,7 +199,6 @@ fn process_instruction( } NESTED_INVOKE => { msg!("nested invoke"); - const ARGUMENT_INDEX: usize = 0; const INVOKED_ARGUMENT_INDEX: usize = 1; const INVOKED_PROGRAM_INDEX: usize = 3; @@ -231,8 +230,10 @@ fn process_instruction( } WRITE_ACCOUNT => { msg!("write account"); + const ARGUMENT_INDEX: usize = 0; + for i in 0..instruction_data[1] { - accounts[0].data.borrow_mut()[i as usize] = instruction_data[1]; + accounts[ARGUMENT_INDEX].data.borrow_mut()[i as usize] = instruction_data[1]; } } _ => panic!(), diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 6ba9c6bb047e9b..91939cef6f6b4e 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -227,7 +227,13 @@ fn run_program( vm.execute_program_jit(&mut instruction_meter) }; assert_eq!(SUCCESS, result.unwrap()); - deserialize_parameters(&bpf_loader::id(), parameter_accounts, ¶meter_bytes).unwrap(); + deserialize_parameters( + &bpf_loader::id(), + parameter_accounts, + ¶meter_bytes, + true, + ) + .unwrap(); if i == 1 { assert_eq!(instruction_count, vm.get_total_instruction_count()); } @@ -736,7 +742,6 @@ fn test_program_bpf_invoke_sanity() { const TEST_RETURN_ERROR: u8 = 11; const TEST_PRIVILEGE_DEESCALATION_ESCALATION_SIGNER: u8 = 12; const TEST_PRIVILEGE_DEESCALATION_ESCALATION_WRITABLE: u8 = 13; - const TEST_WRITE_DEESCALATION: u8 = 14; #[allow(dead_code)] #[derive(Debug)] @@ -854,6 +859,7 @@ fn test_program_bpf_invoke_sanity() { invoked_program_id.clone(), invoked_program_id.clone(), invoked_program_id.clone(), + invoked_program_id.clone(), ], Languages::Rust => vec![ solana_sdk::system_program::id(), @@ -872,6 +878,7 @@ fn test_program_bpf_invoke_sanity() { invoked_program_id.clone(), invoked_program_id.clone(), invoked_program_id.clone(), + invoked_program_id.clone(), ], }; assert_eq!(invoked_programs.len(), expected_invoked_programs.len()); @@ -976,12 +983,6 @@ fn test_program_bpf_invoke_sanity() { &[invoked_program_id.clone()], ); - do_invoke_failure_test_local( - TEST_WRITE_DEESCALATION, - TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified), - &[invoked_program_id.clone()], - ); - // Check resulting state assert_eq!(43, bank.get_balance(&derived_key1)); diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 5a68ad72f81d29..c79d028c3282b3 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -30,6 +30,7 @@ use solana_sdk::{ bpf_loader_upgradeable::{self, UpgradeableLoaderState}, clock::Clock, entrypoint::SUCCESS, + feature_set::skip_ro_deserialization, ic_logger_msg, ic_msg, instruction::InstructionError, keyed_account::{from_keyed_account, next_keyed_account, KeyedAccount}, @@ -818,7 +819,12 @@ impl Executor for BpfExecutor { execute_time.stop(); } let mut deserialize_time = Measure::start("deserialize"); - deserialize_parameters(loader_id, parameter_accounts, ¶meter_bytes)?; + deserialize_parameters( + loader_id, + parameter_accounts, + ¶meter_bytes, + invoke_context.is_feature_active(&skip_ro_deserialization::id()), + )?; deserialize_time.stop(); invoke_context.update_timing( serialize_time.as_us(), diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index f55b100851c097..b3389995042e9b 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -39,14 +39,32 @@ pub fn deserialize_parameters( loader_id: &Pubkey, keyed_accounts: &[KeyedAccount], buffer: &[u8], + skip_ro_deserialization: bool, ) -> Result<(), InstructionError> { if *loader_id == bpf_loader_deprecated::id() { - deserialize_parameters_unaligned(keyed_accounts, buffer) + deserialize_parameters_unaligned(keyed_accounts, buffer, skip_ro_deserialization) } else { - deserialize_parameters_aligned(keyed_accounts, buffer) + deserialize_parameters_aligned(keyed_accounts, buffer, skip_ro_deserialization) } } +pub fn get_serialized_account_size_unaligned( + keyed_account: &KeyedAccount, +) -> Result { + let data_len = keyed_account.data_len()?; + Ok( + size_of::() // is_signer + + size_of::() // is_writable + + size_of::() // key + + size_of::() // lamports + + size_of::() // data len + + data_len // data + + size_of::() // owner + + size_of::() // executable + + size_of::(), // rent_epoch + ) +} + pub fn serialize_parameters_unaligned( program_id: &Pubkey, keyed_accounts: &[KeyedAccount], @@ -56,22 +74,14 @@ pub fn serialize_parameters_unaligned( let mut size = size_of::(); for (i, keyed_account) in keyed_accounts.iter().enumerate() { let (is_dup, _) = is_dup(&keyed_accounts[..i], keyed_account); - size += 1; // dup, signer, writable, executable + size += 1; // dup if !is_dup { - let data_len = keyed_account.data_len()?; - size += size_of::() // key - + size_of::() // owner - + size_of::() // lamports - + size_of::() // data len - + data_len - + MAX_PERMITTED_DATA_INCREASE - + (data_len as *const u8).align_offset(align_of::()) - + size_of::(); // rent epoch; + size += get_serialized_account_size_unaligned(keyed_account)?; } } - size += size_of::() // data len - + instruction_data.len() - + size_of::(); // program id; + size += size_of::() // instruction data len + + instruction_data.len() // instruction data + + size_of::(); // program id let mut v: Vec = Vec::with_capacity(size); v.write_u64::(keyed_accounts.len() as u64) @@ -108,33 +118,58 @@ pub fn serialize_parameters_unaligned( pub fn deserialize_parameters_unaligned( keyed_accounts: &[KeyedAccount], buffer: &[u8], + skip_ro_deserialization: bool, ) -> Result<(), InstructionError> { let mut start = size_of::(); // number of accounts for (i, keyed_account) in keyed_accounts.iter().enumerate() { let (is_dup, _) = is_dup(&keyed_accounts[..i], keyed_account); start += 1; // is_dup if !is_dup { - start += size_of::(); // is_signer - start += size_of::(); // is_writable - start += size_of::(); // pubkey - keyed_account.try_account_ref_mut()?.lamports = - LittleEndian::read_u64(&buffer[start..]); - start += size_of::() // lamports + if keyed_account.is_writable() || !skip_ro_deserialization { + start += size_of::(); // is_signer + start += size_of::(); // is_writable + start += size_of::(); // key + keyed_account.try_account_ref_mut()?.lamports = + LittleEndian::read_u64(&buffer[start..]); + start += size_of::() // lamports + size_of::(); // data length - let end = start + keyed_account.data_len()?; - keyed_account - .try_account_ref_mut()? - .data_as_mut_slice() - .clone_from_slice(&buffer[start..end]); - start += keyed_account.data_len()? // data + let end = start + keyed_account.data_len()?; + keyed_account + .try_account_ref_mut()? + .data_as_mut_slice() + .clone_from_slice(&buffer[start..end]); + start += keyed_account.data_len()? // data + size_of::() // owner + size_of::() // executable + size_of::(); // rent_epoch + } else { + start += get_serialized_account_size_unaligned(keyed_account)?; + } } } Ok(()) } +pub fn get_serialized_account_size_aligned( + keyed_account: &KeyedAccount, +) -> Result { + let data_len = keyed_account.data_len()?; + Ok( + size_of::() // is_signer + + size_of::() // is_writable + + size_of::() // executable + + 4 // padding to 128-bit aligned + + size_of::() // key + + size_of::() // owner + + size_of::() // lamports + + size_of::() // data len + + data_len + + MAX_PERMITTED_DATA_INCREASE + + (data_len as *const u8).align_offset(align_of::()) + + size_of::(), // rent epoch + ) +} + pub fn serialize_parameters_aligned( program_id: &Pubkey, keyed_accounts: &[KeyedAccount], @@ -144,17 +179,11 @@ pub fn serialize_parameters_aligned( let mut size = size_of::(); for (i, keyed_account) in keyed_accounts.iter().enumerate() { let (is_dup, _) = is_dup(&keyed_accounts[..i], keyed_account); - size += 8; // dup, signer, writable, executable - if !is_dup { - let data_len = keyed_account.data_len()?; - size += size_of::() // key - + size_of::() // owner - + size_of::() // lamports - + size_of::() // data len - + data_len - + MAX_PERMITTED_DATA_INCREASE - + (data_len as *const u8).align_offset(align_of::()) - + size_of::(); // rent epoch; + size += 1; // dup + if is_dup { + size += 7; // padding to 64-bit aligned + } else { + size += get_serialized_account_size_aligned(keyed_account)?; } } size += size_of::() // data len @@ -208,6 +237,7 @@ pub fn serialize_parameters_aligned( pub fn deserialize_parameters_aligned( keyed_accounts: &[KeyedAccount], buffer: &[u8], + skip_ro_deserialization: bool, ) -> Result<(), InstructionError> { let mut start = size_of::(); // number of accounts for (i, keyed_account) in keyed_accounts.iter().enumerate() { @@ -215,7 +245,7 @@ pub fn deserialize_parameters_aligned( start += size_of::(); // position if is_dup { start += 7; // padding to 64-bit aligned - } else { + } else if keyed_account.is_writable() || !skip_ro_deserialization { let mut account = keyed_account.try_account_ref_mut()?; start += size_of::() // is_signer + size_of::() // is_writable @@ -242,6 +272,8 @@ pub fn deserialize_parameters_aligned( start += pre_len + MAX_PERMITTED_DATA_INCREASE; // data start += (start as *const u8).align_offset(align_of::()); start += size_of::(); // rent_epoch + } else { + start += get_serialized_account_size_aligned(keyed_account)?; } } Ok(()) @@ -267,11 +299,16 @@ mod tests { fn test_serialize_parameters() { let program_id = solana_sdk::pubkey::new_rand(); let dup_key = solana_sdk::pubkey::new_rand(); + let dup_key2 = solana_sdk::pubkey::new_rand(); let keys = vec![ dup_key, dup_key, solana_sdk::pubkey::new_rand(), solana_sdk::pubkey::new_rand(), + dup_key2, + dup_key2, + solana_sdk::pubkey::new_rand(), + solana_sdk::pubkey::new_rand(), ]; let accounts = [ RefCell::new(AccountSharedData::from(Account { @@ -281,7 +318,7 @@ mod tests { executable: false, rent_epoch: 100, })), - // dup of first + // dup RefCell::new(AccountSharedData::from(Account { lamports: 1, data: vec![1u8, 2, 3, 4, 5], @@ -303,12 +340,48 @@ mod tests { executable: false, rent_epoch: 3100, })), + RefCell::new(AccountSharedData::from(Account { + lamports: 4, + data: vec![1u8, 2, 3, 4, 5], + owner: bpf_loader::id(), + executable: false, + rent_epoch: 100, + })), + // dup + RefCell::new(AccountSharedData::from(Account { + lamports: 4, + data: vec![1u8, 2, 3, 4, 5], + owner: bpf_loader::id(), + executable: false, + rent_epoch: 100, + })), + RefCell::new(AccountSharedData::from(Account { + lamports: 5, + data: vec![11u8, 12, 13, 14, 15, 16, 17, 18, 19], + owner: bpf_loader::id(), + executable: true, + rent_epoch: 200, + })), + RefCell::new(AccountSharedData::from(Account { + lamports: 6, + data: vec![], + owner: bpf_loader::id(), + executable: false, + rent_epoch: 3100, + })), ]; let keyed_accounts: Vec<_> = keys .iter() .zip(&accounts) - .map(|(key, account)| KeyedAccount::new(&key, false, &account)) + .enumerate() + .map(|(i, (key, account))| { + if i <= accounts.len() / 2 { + KeyedAccount::new_readonly(&key, false, &account) + } else { + KeyedAccount::new(&key, false, &account) + } + }) .collect(); let instruction_data = vec![1u8, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]; @@ -321,9 +394,9 @@ mod tests { &instruction_data, ) .unwrap(); + let (de_program_id, de_accounts, de_instruction_data) = unsafe { deserialize(&mut serialized[0] as *mut u8) }; - assert_eq!(&program_id, de_program_id); assert_eq!(instruction_data, de_instruction_data); assert_eq!( @@ -353,6 +426,35 @@ mod tests { ); } + let de_accounts = accounts.clone(); + let de_keyed_accounts: Vec<_> = keys + .iter() + .zip(&de_accounts) + .enumerate() + .map(|(i, (key, account))| { + if i <= accounts.len() / 2 { + KeyedAccount::new_readonly(&key, false, &account) + } else { + KeyedAccount::new(&key, false, &account) + } + }) + .collect(); + deserialize_parameters(&bpf_loader::id(), &de_keyed_accounts, &serialized, true).unwrap(); + for ((account, de_keyed_account), key) in + accounts.iter().zip(de_keyed_accounts).zip(keys.clone()) + { + assert_eq!(key, *de_keyed_account.unsigned_key()); + let account = account.borrow(); + assert_eq!(account.lamports, de_keyed_account.lamports().unwrap()); + assert_eq!( + &account.data()[..], + &de_keyed_account.try_account_ref().unwrap().data[..] + ); + assert_eq!(account.owner, de_keyed_account.owner().unwrap()); + assert_eq!(account.executable, de_keyed_account.executable().unwrap()); + assert_eq!(account.rent_epoch, de_keyed_account.rent_epoch().unwrap()); + } + // check serialize_parameters_unaligned let mut serialized = serialize_parameters( @@ -362,12 +464,12 @@ mod tests { &instruction_data, ) .unwrap(); + let (de_program_id, de_accounts, de_instruction_data) = unsafe { deserialize_unaligned(&mut serialized[0] as *mut u8) }; - assert_eq!(&program_id, de_program_id); assert_eq!(instruction_data, de_instruction_data); - for ((account, account_info), key) in accounts.iter().zip(de_accounts).zip(keys) { + for ((account, account_info), key) in accounts.iter().zip(de_accounts).zip(keys.clone()) { assert_eq!(key, *account_info.key); let account = account.borrow(); assert_eq!(account.lamports, account_info.lamports()); @@ -376,6 +478,41 @@ mod tests { assert_eq!(account.executable, account_info.executable); assert_eq!(account.rent_epoch, account_info.rent_epoch); } + + let de_accounts = accounts.clone(); + let de_keyed_accounts: Vec<_> = keys + .iter() + .zip(&de_accounts) + .enumerate() + .map(|(i, (key, account))| { + if i < accounts.len() / 2 { + KeyedAccount::new_readonly(&key, false, &account) + } else { + KeyedAccount::new(&key, false, &account) + } + }) + .collect(); + deserialize_parameters( + &bpf_loader_deprecated::id(), + &de_keyed_accounts, + &serialized, + true, + ) + .unwrap(); + for ((account, de_keyed_account), key) in + accounts.iter().zip(de_keyed_accounts).zip(keys.clone()) + { + assert_eq!(key, *de_keyed_account.unsigned_key()); + let account = account.borrow(); + assert_eq!(account.lamports, de_keyed_account.lamports().unwrap()); + assert_eq!( + &account.data()[..], + &de_keyed_account.try_account_ref().unwrap().data[..] + ); + assert_eq!(account.owner, de_keyed_account.owner().unwrap()); + assert_eq!(account.executable, de_keyed_account.executable().unwrap()); + assert_eq!(account.rent_epoch, de_keyed_account.rent_epoch().unwrap()); + } } // the old bpf_loader in-program deserializer bpf_loader::id() diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 1bb20423f90423..0d6a30808033fc 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -107,6 +107,10 @@ pub mod cpi_share_ro_and_exec_accounts { solana_sdk::declare_id!("6VgVBi3uRVqp56TtEwNou8idgdmhCD1aYqX8FaJ1fnJb"); } +pub mod skip_ro_deserialization { + solana_sdk::declare_id!("6Sw5JV84f7QkDe8gvRxpcPWFnPpfpgEnNziiy8sELaCp"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -134,6 +138,7 @@ lazy_static! { (check_init_vote_data::id(), "check initialized Vote data"), (check_program_owner::id(), "limit programs to operating on accounts owned by itself"), (cpi_share_ro_and_exec_accounts::id(), "Share RO and Executable accounts during cross-program invocations"), + (skip_ro_deserialization::id(), "Skip deserialization of read-only accounts"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()