From b168cadeea30f802914f077ed433c02a9fe2e65b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Wed, 15 Nov 2023 20:58:10 +0100 Subject: [PATCH] Cleanup - Feature gate of `check_slice_translation_size` (#34084) Cleans up feature gate of check_slice_translation_size. --- program-runtime/src/invoke_context.rs | 8 +- programs/bpf_loader/src/syscalls/cpi.rs | 32 +------- programs/bpf_loader/src/syscalls/logging.rs | 3 - programs/bpf_loader/src/syscalls/mem_ops.rs | 5 -- programs/bpf_loader/src/syscalls/mod.rs | 84 ++++----------------- 5 files changed, 18 insertions(+), 114 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index eaee493bbfb9d7..bdb870a02c1dda 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -20,7 +20,7 @@ use { solana_sdk::{ account::AccountSharedData, bpf_loader_deprecated, - feature_set::{check_slice_translation_size, native_programs_consume_cu, FeatureSet}, + feature_set::{native_programs_consume_cu, FeatureSet}, hash::Hash, instruction::{AccountMeta, InstructionError}, native_loader, @@ -599,12 +599,6 @@ impl<'a> InvokeContext<'a> { .unwrap_or(true) } - // Set should type size be checked during user pointer translation - pub fn get_check_size(&self) -> bool { - self.feature_set - .is_active(&check_slice_translation_size::id()) - } - // Set this instruction syscall context pub fn set_syscall_context( &mut self, diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 90b4cf07280bf2..7db291d384fa85 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -239,7 +239,6 @@ impl<'a, 'b> CallerAccount<'a, 'b> { vm_data_addr, data.len() as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )? }; ( @@ -344,7 +343,6 @@ impl<'a, 'b> CallerAccount<'a, 'b> { account_info.data_addr, account_info.data_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )? }; @@ -498,7 +496,6 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { ix.accounts.as_ptr() as u64, ix.accounts.len() as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let accounts = if invoke_context .feature_set @@ -542,7 +539,6 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { ix.data.as_ptr() as u64, ix_data_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )? .to_vec(); @@ -597,7 +593,6 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { signers_seeds_addr, signers_seeds_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; if signers_seeds.len() > MAX_SIGNERS { return Err(Box::new(SyscallError::TooManySigners)); @@ -608,7 +603,6 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { signer_seeds.as_ptr() as *const _ as u64, signer_seeds.len() as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; if untranslated_seeds.len() > MAX_SEEDS { return Err(Box::new(InstructionError::MaxSeedLengthExceeded)); @@ -621,7 +615,6 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { untranslated_seed.as_ptr() as *const _ as u64, untranslated_seed.len() as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), ) }) .collect::, Error>>()?; @@ -740,7 +733,6 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { ix_c.accounts_addr, ix_c.accounts_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let ix_data_len = ix_c.data_len; @@ -761,7 +753,6 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { ix_c.data_addr, ix_data_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )? .to_vec(); @@ -862,7 +853,6 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { signers_seeds_addr, signers_seeds_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; if signers_seeds.len() > MAX_SIGNERS { return Err(Box::new(SyscallError::TooManySigners)); @@ -875,7 +865,6 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { signer_seeds.addr, signer_seeds.len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; if seeds.len() > MAX_SEEDS { return Err(Box::new(InstructionError::MaxSeedLengthExceeded) as Error); @@ -888,7 +877,6 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { seed.addr, seed.len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), ) }) .collect::, Error>>()?; @@ -917,7 +905,6 @@ where account_infos_addr, account_infos_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; check_account_infos(account_infos.len(), invoke_context)?; let account_info_keys = if invoke_context @@ -1336,7 +1323,6 @@ fn update_callee_account( .saturating_add(caller_account.original_data_len as u64), realloc_bytes_used as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; callee_account .get_data_mut()? @@ -1581,7 +1567,6 @@ fn update_caller_account( .saturating_add(dirty_realloc_start as u64), dirty_realloc_len as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; serialized_data.fill(0); } @@ -1602,7 +1587,6 @@ fn update_caller_account( caller_account.vm_data_addr, post_len as u64, false, // Don't care since it is byte aligned - invoke_context.get_check_size(), )?; } // this is the len field in the AccountInfo::data slice @@ -1666,7 +1650,6 @@ fn update_caller_account( .saturating_add(caller_account.original_data_len as u64), realloc_bytes_used as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )? }; let from_slice = callee_account @@ -2182,15 +2165,9 @@ mod tests { // check that the caller account data pointer always matches the callee account data pointer assert_eq!( - translate_slice::( - &memory_mapping, - caller_account.vm_data_addr, - 1, - true, - true - ) - .unwrap() - .as_ptr(), + translate_slice::(&memory_mapping, caller_account.vm_data_addr, 1, true,) + .unwrap() + .as_ptr(), callee_account.get_data().as_ptr() ); @@ -2210,7 +2187,6 @@ mod tests { .saturating_add(caller_account.original_data_len as u64), MAX_PERMITTED_DATA_INCREASE as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), ) .unwrap(); @@ -2360,7 +2336,6 @@ mod tests { caller_account.vm_data_addr, callee_account.get_data().len() as u64, true, - true, ) .unwrap(); assert_eq!(data, callee_account.get_data()); @@ -2632,7 +2607,6 @@ mod tests { .saturating_add(caller_account.original_data_len as u64), 3, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), ) .unwrap(); serialized_data.copy_from_slice(b"baz"); diff --git a/programs/bpf_loader/src/syscalls/logging.rs b/programs/bpf_loader/src/syscalls/logging.rs index c5faf0a1057fde..fd3994fd88ee75 100644 --- a/programs/bpf_loader/src/syscalls/logging.rs +++ b/programs/bpf_loader/src/syscalls/logging.rs @@ -23,7 +23,6 @@ declare_builtin_function!( addr, len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), invoke_context .feature_set .is_active(&stop_truncating_strings_in_syscalls::id()), @@ -129,7 +128,6 @@ declare_builtin_function!( addr, len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; consume_compute_meter( @@ -153,7 +151,6 @@ declare_builtin_function!( untranslated_field.as_ptr() as *const _ as u64, untranslated_field.len() as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?); } diff --git a/programs/bpf_loader/src/syscalls/mem_ops.rs b/programs/bpf_loader/src/syscalls/mem_ops.rs index 9354270ac2f0b7..7e9b69fc6f310c 100644 --- a/programs/bpf_loader/src/syscalls/mem_ops.rs +++ b/programs/bpf_loader/src/syscalls/mem_ops.rs @@ -84,14 +84,12 @@ declare_builtin_function!( s1_addr, n, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let s2 = translate_slice::( memory_mapping, s2_addr, n, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let cmp_result = translate_type_mut::( memory_mapping, @@ -137,7 +135,6 @@ declare_builtin_function!( dst_addr, n, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; s.fill(c as u8); Ok(0) @@ -163,7 +160,6 @@ fn memmove( dst_addr, n, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )? .as_mut_ptr(); let src_ptr = translate_slice::( @@ -171,7 +167,6 @@ fn memmove( src_addr, n, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )? .as_ptr(); diff --git a/programs/bpf_loader/src/syscalls/mod.rs b/programs/bpf_loader/src/syscalls/mod.rs index 2c5da75c8742d7..a10693bb9fc91a 100644 --- a/programs/bpf_loader/src/syscalls/mod.rs +++ b/programs/bpf_loader/src/syscalls/mod.rs @@ -514,14 +514,13 @@ fn translate_slice_inner<'a, T>( vm_addr: u64, len: u64, check_aligned: bool, - check_size: bool, ) -> Result<&'a mut [T], Error> { if len == 0 { return Ok(&mut []); } let total_size = len.saturating_mul(size_of::() as u64); - if check_size && isize::try_from(total_size).is_err() { + if isize::try_from(total_size).is_err() { return Err(SyscallError::InvalidLength.into()); } @@ -537,7 +536,6 @@ fn translate_slice_mut<'a, T>( vm_addr: u64, len: u64, check_aligned: bool, - check_size: bool, ) -> Result<&'a mut [T], Error> { translate_slice_inner::( memory_mapping, @@ -545,7 +543,6 @@ fn translate_slice_mut<'a, T>( vm_addr, len, check_aligned, - check_size, ) } fn translate_slice<'a, T>( @@ -553,7 +550,6 @@ fn translate_slice<'a, T>( vm_addr: u64, len: u64, check_aligned: bool, - check_size: bool, ) -> Result<&'a [T], Error> { translate_slice_inner::( memory_mapping, @@ -561,7 +557,6 @@ fn translate_slice<'a, T>( vm_addr, len, check_aligned, - check_size, ) .map(|value| &*value) } @@ -573,11 +568,10 @@ fn translate_string_and_do( addr: u64, len: u64, check_aligned: bool, - check_size: bool, stop_truncating_strings_in_syscalls: bool, work: &mut dyn FnMut(&str) -> Result, ) -> Result { - let buf = translate_slice::(memory_mapping, addr, len, check_aligned, check_size)?; + let buf = translate_slice::(memory_mapping, addr, len, check_aligned)?; let msg = if stop_truncating_strings_in_syscalls { buf } else { @@ -632,7 +626,6 @@ declare_builtin_function!( file, len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), invoke_context .feature_set .is_active(&stop_truncating_strings_in_syscalls::id()), @@ -685,15 +678,9 @@ fn translate_and_check_program_address_inputs<'a>( program_id_addr: u64, memory_mapping: &mut MemoryMapping, check_aligned: bool, - check_size: bool, ) -> Result<(Vec<&'a [u8]>, &'a Pubkey), Error> { - let untranslated_seeds = translate_slice::<&[u8]>( - memory_mapping, - seeds_addr, - seeds_len, - check_aligned, - check_size, - )?; + let untranslated_seeds = + translate_slice::<&[u8]>(memory_mapping, seeds_addr, seeds_len, check_aligned)?; if untranslated_seeds.len() > MAX_SEEDS { return Err(SyscallError::BadSeeds(PubkeyError::MaxSeedLengthExceeded).into()); } @@ -708,7 +695,6 @@ fn translate_and_check_program_address_inputs<'a>( untranslated_seed.as_ptr() as *const _ as u64, untranslated_seed.len() as u64, check_aligned, - check_size, ) }) .collect::, Error>>()?; @@ -739,7 +725,6 @@ declare_builtin_function!( program_id_addr, memory_mapping, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let Ok(new_address) = Pubkey::create_program_address(&seeds, program_id) else { @@ -750,7 +735,6 @@ declare_builtin_function!( address_addr, 32, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; address.copy_from_slice(new_address.as_ref()); Ok(0) @@ -780,7 +764,6 @@ declare_builtin_function!( program_id_addr, memory_mapping, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let mut bump_seed = [std::u8::MAX]; @@ -802,7 +785,6 @@ declare_builtin_function!( address_addr, std::mem::size_of::() as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; if !is_nonoverlapping( bump_seed_ref as *const _ as usize, @@ -844,21 +826,18 @@ declare_builtin_function!( hash_addr, keccak::HASH_BYTES as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let signature = translate_slice::( memory_mapping, signature_addr, SECP256K1_SIGNATURE_LENGTH as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let secp256k1_recover_result = translate_slice_mut::( memory_mapping, result_addr, SECP256K1_PUBLIC_KEY_LENGTH as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let Ok(message) = libsecp256k1::Message::parse_slice(hash) else { @@ -1188,7 +1167,6 @@ declare_builtin_function!( scalars_addr, points_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let points = translate_slice::( @@ -1196,7 +1174,6 @@ declare_builtin_function!( points_addr, points_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; if let Some(result_point) = edwards::multiscalar_multiply_edwards(scalars, points) { @@ -1228,7 +1205,6 @@ declare_builtin_function!( scalars_addr, points_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let points = translate_slice::( @@ -1236,7 +1212,6 @@ declare_builtin_function!( points_addr, points_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; if let Some(result_point) = @@ -1290,7 +1265,6 @@ declare_builtin_function!( addr, len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )? .to_vec() }; @@ -1337,7 +1311,6 @@ declare_builtin_function!( return_data_addr, length, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let to_slice = return_data_result; @@ -1439,14 +1412,12 @@ declare_builtin_function!( data_addr, result_header.data_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let accounts = translate_slice_mut::( memory_mapping, accounts_addr, result_header.accounts_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; if !is_nonoverlapping( @@ -1589,7 +1560,6 @@ declare_builtin_function!( input_addr, input_size, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let call_result = translate_slice_mut::( @@ -1597,7 +1567,6 @@ declare_builtin_function!( result_addr, output as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let calculation = match group_op { @@ -1642,7 +1611,6 @@ declare_builtin_function!( params, 1, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )? .first() .ok_or(SyscallError::InvalidLength)?; @@ -1670,7 +1638,6 @@ declare_builtin_function!( params.base as *const _ as u64, params.base_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let exponent = translate_slice::( @@ -1678,7 +1645,6 @@ declare_builtin_function!( params.exponent as *const _ as u64, params.exponent_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let modulus = translate_slice::( @@ -1686,7 +1652,6 @@ declare_builtin_function!( params.modulus as *const _ as u64, params.modulus_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let value = big_mod_exp(base, exponent, modulus); @@ -1696,7 +1661,6 @@ declare_builtin_function!( return_value, params.modulus_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; return_value.copy_from_slice(value.as_slice()); @@ -1743,14 +1707,12 @@ declare_builtin_function!( result_addr, poseidon::HASH_BYTES as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let inputs = translate_slice::<&[u8]>( memory_mapping, vals_addr, vals_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let inputs = inputs .iter() @@ -1760,7 +1722,6 @@ declare_builtin_function!( input.as_ptr() as *const _ as u64, input.len() as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), ) }) .collect::, Error>>()?; @@ -1842,7 +1803,6 @@ declare_builtin_function!( input_addr, input_size, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let call_result = translate_slice_mut::( @@ -1850,7 +1810,6 @@ declare_builtin_function!( result_addr, output as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; match op { @@ -1933,7 +1892,6 @@ declare_builtin_function!( result_addr, std::mem::size_of::() as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let mut hasher = H::create_hasher(); if vals_len > 0 { @@ -1942,7 +1900,6 @@ declare_builtin_function!( vals_addr, vals_len, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; for val in vals.iter() { let bytes = translate_slice::( @@ -1950,7 +1907,6 @@ declare_builtin_function!( val.as_ptr() as u64, val.len() as u64, invoke_context.get_check_aligned(), - invoke_context.get_check_size(), )?; let cost = compute_budget.mem_op_base_cost.max( hash_byte_cost.saturating_mul( @@ -2130,7 +2086,7 @@ mod tests { ) .unwrap(); let translated_data = - translate_slice::(&memory_mapping, data.as_ptr() as u64, 0, true, true).unwrap(); + translate_slice::(&memory_mapping, data.as_ptr() as u64, 0, true).unwrap(); assert_eq!(data, translated_data); assert_eq!(0, translated_data.len()); @@ -2143,24 +2099,18 @@ mod tests { ) .unwrap(); let translated_data = - translate_slice::(&memory_mapping, 0x100000000, data.len() as u64, true, true) - .unwrap(); + translate_slice::(&memory_mapping, 0x100000000, data.len() as u64, true).unwrap(); assert_eq!(data, translated_data); *data.first_mut().unwrap() = 10; assert_eq!(data, translated_data); assert!( - translate_slice::(&memory_mapping, data.as_ptr() as u64, u64::MAX, true, true) - .is_err() + translate_slice::(&memory_mapping, data.as_ptr() as u64, u64::MAX, true).is_err() ); - assert!(translate_slice::( - &memory_mapping, - 0x100000000 - 1, - data.len() as u64, - true, - true - ) - .is_err()); + assert!( + translate_slice::(&memory_mapping, 0x100000000 - 1, data.len() as u64, true,) + .is_err() + ); // u64 let mut data = vec![1u64, 2, 3, 4, 5]; @@ -2174,14 +2124,11 @@ mod tests { ) .unwrap(); let translated_data = - translate_slice::(&memory_mapping, 0x100000000, data.len() as u64, true, true) - .unwrap(); + translate_slice::(&memory_mapping, 0x100000000, data.len() as u64, true).unwrap(); assert_eq!(data, translated_data); *data.first_mut().unwrap() = 10; assert_eq!(data, translated_data); - assert!( - translate_slice::(&memory_mapping, 0x100000000, u64::MAX, true, true).is_err() - ); + assert!(translate_slice::(&memory_mapping, 0x100000000, u64::MAX, true).is_err()); // Pubkeys let mut data = vec![solana_sdk::pubkey::new_rand(); 5]; @@ -2197,7 +2144,7 @@ mod tests { ) .unwrap(); let translated_data = - translate_slice::(&memory_mapping, 0x100000000, data.len() as u64, true, true) + translate_slice::(&memory_mapping, 0x100000000, data.len() as u64, true) .unwrap(); assert_eq!(data, translated_data); *data.first_mut().unwrap() = solana_sdk::pubkey::new_rand(); // Both should point to same place @@ -2222,7 +2169,6 @@ mod tests { string.len() as u64, true, true, - true, &mut |string: &str| { assert_eq!(string, "Gaggablaghblagh!"); Ok(42) @@ -3703,7 +3649,6 @@ mod tests { VM_BASE_ADDRESS.saturating_add(DATA_OFFSET as u64), processed_sibling_instruction.data_len, true, - true, ) .unwrap(); let accounts = translate_slice_mut::( @@ -3711,7 +3656,6 @@ mod tests { VM_BASE_ADDRESS.saturating_add(ACCOUNTS_OFFSET as u64), processed_sibling_instruction.accounts_len, true, - true, ) .unwrap();