From e9912744ef86cf851f47901fcd7e27b167dbc096 Mon Sep 17 00:00:00 2001 From: Jack May Date: Wed, 2 Mar 2022 14:50:16 -0800 Subject: [PATCH] use saturating arithmetic (#23435) --- programs/bpf_loader/src/allocator_bump.rs | 2 + programs/bpf_loader/src/lib.rs | 123 ++++++++++++++-------- programs/bpf_loader/src/serialization.rs | 2 + programs/bpf_loader/src/syscalls.rs | 89 ++++++++++------ 4 files changed, 138 insertions(+), 78 deletions(-) diff --git a/programs/bpf_loader/src/allocator_bump.rs b/programs/bpf_loader/src/allocator_bump.rs index 227c88f47c..a4f97b2b3c 100644 --- a/programs/bpf_loader/src/allocator_bump.rs +++ b/programs/bpf_loader/src/allocator_bump.rs @@ -1,3 +1,5 @@ +#![allow(clippy::integer_arithmetic)] + use { crate::alloc, alloc::{Alloc, AllocErr}, diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 12e992f283..0e0de2e4f0 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -1,4 +1,3 @@ -#![allow(clippy::integer_arithmetic)] pub mod alloc; pub mod allocator_bump; pub mod deprecated; @@ -192,16 +191,17 @@ fn write_program_data<'a>( log_collector: Option>>, ) -> Result<(), InstructionError> { let data = program.get_data_mut(); - if data.len() < program_data_offset + bytes.len() { + let write_offset = program_data_offset.saturating_add(bytes.len()); + if data.len() < write_offset { ic_logger_msg!( log_collector, "Write overflow: {} < {}", data.len(), - program_data_offset + bytes.len() + write_offset ); return Err(InstructionError::AccountDataTooSmall); } - data[program_data_offset..program_data_offset + bytes.len()].copy_from_slice(bytes); + data[program_data_offset..write_offset].copy_from_slice(bytes); Ok(()) } @@ -224,10 +224,11 @@ pub fn create_vm<'a, 'b>( .feature_set .is_active(&requestable_heap_size::id()) { - let _ = invoke_context - .get_compute_meter() - .borrow_mut() - .consume((heap_size as u64 / (32 * 1024)).saturating_sub(1) * compute_budget.heap_cost); + let _ = invoke_context.get_compute_meter().borrow_mut().consume( + ((heap_size as u64).saturating_div(32_u64.saturating_mul(1024))) + .saturating_sub(1) + .saturating_mul(compute_budget.heap_cost), + ); } let mut heap = AlignedMemory::new_with_size(compute_budget.heap_size.unwrap_or(HEAP_LENGTH), HOST_ALIGN); @@ -276,7 +277,7 @@ fn process_instruction_common( instruction_context.get_index_in_transaction(first_instruction_account)?, )?; let second_account_key = instruction_context - .get_index_in_transaction(first_instruction_account + 1) + .get_index_in_transaction(first_instruction_account.saturating_add(1)) .and_then(|index_in_transaction| { transaction_context.get_key_of_account_at_index(index_in_transaction) }); @@ -287,7 +288,7 @@ fn process_instruction_common( .map(|key| key == program_id) .unwrap_or(false) { - first_instruction_account + 1 + first_instruction_account.saturating_add(1) } else { if instruction_context .try_borrow_account(transaction_context, first_instruction_account)? @@ -302,9 +303,11 @@ fn process_instruction_common( let program = instruction_context.try_borrow_account(transaction_context, program_account_index)?; if program.is_executable() { + // First instruction account can only be zero if called from CPI, which + // means stack height better be greater than one debug_assert_eq!( - first_instruction_account, - 1 - (invoke_context.get_stack_height() > 1) as usize, + first_instruction_account == 0, + invoke_context.get_stack_height() > 1 ); if !check_loader_id(program.get_owner()) { @@ -507,8 +510,9 @@ fn process_loader_upgradeable_instruction( return Err(InstructionError::IncorrectAuthority); } if !instruction_context.is_signer( - instruction_context.get_number_of_program_accounts() - + upgradeable_ins_acc_idx::Write::Authority as usize, + instruction_context + .get_number_of_program_accounts() + .saturating_add(upgradeable_ins_acc_idx::Write::Authority as usize), )? { ic_logger_msg!(log_collector, "Buffer authority did not sign"); return Err(InstructionError::MissingRequiredSignature); @@ -519,7 +523,7 @@ fn process_loader_upgradeable_instruction( )?; write_program_data( &mut program, - UpgradeableLoaderState::buffer_data_offset()? + offset as usize, + UpgradeableLoaderState::buffer_data_offset()?.saturating_add(offset as usize), &bytes, invoke_context.get_log_collector(), )?; @@ -582,8 +586,12 @@ fn process_loader_upgradeable_instruction( return Err(InstructionError::IncorrectAuthority); } if !instruction_context.is_signer( - instruction_context.get_number_of_program_accounts() - + upgradeable_ins_acc_idx::DeployWithMaxDataLen::UpgradeAuthority as usize, + instruction_context + .get_number_of_program_accounts() + .saturating_add( + upgradeable_ins_acc_idx::DeployWithMaxDataLen::UpgradeAuthority + as usize, + ), )? { ic_logger_msg!(log_collector, "Upgrade authority did not sign"); return Err(InstructionError::MissingRequiredSignature); @@ -662,7 +670,7 @@ fn process_loader_upgradeable_instruction( // Load and verify the program bits let executor = create_executor( - first_instruction_account + 3, + first_instruction_account.saturating_add(3), buffer_data_offset, invoke_context, use_jit, @@ -687,8 +695,8 @@ fn process_loader_upgradeable_instruction( slot: clock.slot, upgrade_authority_address, })?; - programdata.get_data_mut() - [programdata_data_offset..programdata_data_offset + buffer_data_len] + programdata.get_data_mut()[programdata_data_offset + ..programdata_data_offset.saturating_add(buffer_data_len)] .copy_from_slice(&buffer.get_data()[buffer_data_offset..]); } @@ -783,8 +791,11 @@ fn process_loader_upgradeable_instruction( return Err(InstructionError::IncorrectAuthority); } if !instruction_context.is_signer( - instruction_context.get_number_of_program_accounts() - + upgradeable_ins_acc_idx::Upgrade::UpgradeAuthority as usize, + instruction_context + .get_number_of_program_accounts() + .saturating_add( + upgradeable_ins_acc_idx::Upgrade::UpgradeAuthority as usize, + ), )? { ic_logger_msg!(log_collector, "Upgrade authority did not sign"); return Err(InstructionError::MissingRequiredSignature); @@ -817,7 +828,11 @@ fn process_loader_upgradeable_instruction( ic_logger_msg!(log_collector, "ProgramData account not large enough"); return Err(InstructionError::AccountDataTooSmall); } - if programdata.get_lamports() + buffer.get_lamports() < programdata_balance_required { + if programdata + .get_lamports() + .saturating_add(buffer.get_lamports()) + < programdata_balance_required + { ic_logger_msg!( log_collector, "Buffer account balance too low to fund upgrade" @@ -839,8 +854,11 @@ fn process_loader_upgradeable_instruction( return Err(InstructionError::IncorrectAuthority); } if !instruction_context.is_signer( - instruction_context.get_number_of_program_accounts() - + upgradeable_ins_acc_idx::Upgrade::UpgradeAuthority as usize, + instruction_context + .get_number_of_program_accounts() + .saturating_add( + upgradeable_ins_acc_idx::Upgrade::UpgradeAuthority as usize, + ), )? { ic_logger_msg!(log_collector, "Upgrade authority did not sign"); return Err(InstructionError::MissingRequiredSignature); @@ -853,7 +871,7 @@ fn process_loader_upgradeable_instruction( // Load and verify the program bits let executor = create_executor( - first_instruction_account + 2, + first_instruction_account.saturating_add(2), buffer_data_offset, invoke_context, use_jit, @@ -878,9 +896,10 @@ fn process_loader_upgradeable_instruction( upgrade_authority_address: current_upgrade_authority_address, })?; programdata.get_data_mut() - [programdata_data_offset..programdata_data_offset + buffer_data_len] + [programdata_data_offset..programdata_data_offset.saturating_add(buffer_data_len)] .copy_from_slice(&buffer.get_data()[buffer_data_offset..]); - programdata.get_data_mut()[programdata_data_offset + buffer_data_len..].fill(0); + programdata.get_data_mut()[programdata_data_offset.saturating_add(buffer_data_len)..] + .fill(0); // Fund ProgramData to rent-exemption, spill the rest { @@ -889,8 +908,10 @@ fn process_loader_upgradeable_instruction( upgradeable_ins_acc_idx::Upgrade::Spill as usize, )?; spill.checked_add_lamports( - (programdata.get_lamports() + buffer.get_lamports()) - .saturating_sub(programdata_balance_required), + (programdata + .get_lamports() + .saturating_add(buffer.get_lamports())) + .saturating_sub(programdata_balance_required), )?; buffer.set_lamports(0); programdata.set_lamports(programdata_balance_required); @@ -931,8 +952,11 @@ fn process_loader_upgradeable_instruction( return Err(InstructionError::IncorrectAuthority); } if !instruction_context.is_signer( - instruction_context.get_number_of_program_accounts() - + upgradeable_ins_acc_idx::SetAuthority::CurrentAuthority as usize, + instruction_context + .get_number_of_program_accounts() + .saturating_add( + upgradeable_ins_acc_idx::SetAuthority::CurrentAuthority as usize, + ), )? { ic_logger_msg!(log_collector, "Buffer authority did not sign"); return Err(InstructionError::MissingRequiredSignature); @@ -954,8 +978,11 @@ fn process_loader_upgradeable_instruction( return Err(InstructionError::IncorrectAuthority); } if !instruction_context.is_signer( - instruction_context.get_number_of_program_accounts() - + upgradeable_ins_acc_idx::SetAuthority::CurrentAuthority as usize, + instruction_context + .get_number_of_program_accounts() + .saturating_add( + upgradeable_ins_acc_idx::SetAuthority::CurrentAuthority as usize, + ), )? { ic_logger_msg!(log_collector, "Upgrade authority did not sign"); return Err(InstructionError::MissingRequiredSignature); @@ -976,9 +1003,11 @@ fn process_loader_upgradeable_instruction( UpgradeableLoaderInstruction::Close => { instruction_context.check_number_of_instruction_accounts(2)?; if instruction_context.get_index_in_transaction( - first_instruction_account + upgradeable_ins_acc_idx::Close::Account as usize, + first_instruction_account + .saturating_add(upgradeable_ins_acc_idx::Close::Account as usize), )? == instruction_context.get_index_in_transaction( - first_instruction_account + upgradeable_ins_acc_idx::Close::Recipient as usize, + first_instruction_account + .saturating_add(upgradeable_ins_acc_idx::Close::Recipient as usize), )? { ic_logger_msg!( log_collector, @@ -1029,8 +1058,9 @@ fn process_loader_upgradeable_instruction( } => { instruction_context.check_number_of_instruction_accounts(4)?; if !instruction_context.is_writable( - instruction_context.get_number_of_program_accounts() - + upgradeable_ins_acc_idx::Close::Program as usize, + instruction_context + .get_number_of_program_accounts() + .saturating_add(upgradeable_ins_acc_idx::Close::Program as usize), )? { ic_logger_msg!(log_collector, "Program account is not writable"); return Err(InstructionError::InvalidArgument); @@ -1105,8 +1135,9 @@ fn common_close_account( return Err(InstructionError::IncorrectAuthority); } if !instruction_context.is_signer( - instruction_context.get_number_of_program_accounts() - + upgradeable_ins_acc_idx::Close::Authority as usize, + instruction_context + .get_number_of_program_accounts() + .saturating_add(upgradeable_ins_acc_idx::Close::Authority as usize), )? { ic_logger_msg!(log_collector, "Authority did not sign"); return Err(InstructionError::MissingRequiredSignature); @@ -1158,8 +1189,9 @@ fn process_loader_instruction( match limited_deserialize(instruction_data)? { LoaderInstruction::Write { offset, bytes } => { if !instruction_context.is_signer( - instruction_context.get_number_of_program_accounts() - + deprecated_ins_acc_idx::Write::Program as usize, + instruction_context + .get_number_of_program_accounts() + .saturating_add(deprecated_ins_acc_idx::Write::Program as usize), )? { ic_msg!(invoke_context, "Program account did not sign"); return Err(InstructionError::MissingRequiredSignature); @@ -1176,8 +1208,9 @@ fn process_loader_instruction( } LoaderInstruction::Finalize => { if !instruction_context.is_signer( - instruction_context.get_number_of_program_accounts() - + deprecated_ins_acc_idx::Finalize::Program as usize, + instruction_context + .get_number_of_program_accounts() + .saturating_add(deprecated_ins_acc_idx::Finalize::Program as usize), )? { ic_msg!(invoke_context, "Program account did not sign"); return Err(InstructionError::MissingRequiredSignature); @@ -1281,7 +1314,7 @@ impl Executor for BpfExecutor { log_collector, "Program {} consumed {} of {} compute units", &program_id, - before - after, + before.saturating_sub(after), before ); if log_enabled!(Trace) { diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index d269b8a1f6..bc9094fdb5 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -1,3 +1,5 @@ +#![allow(clippy::integer_arithmetic)] + use { byteorder::{ByteOrder, LittleEndian, WriteBytesExt}, solana_rbpf::{aligned_memory::AlignedMemory, ebpf::HOST_ALIGN}, diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index 47d8b91b1b..de47cbaa12 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -1037,7 +1037,7 @@ impl<'a, 'b> SyscallObject for SyscallTryFindProgramAddress<'a, 'b> { return; } } - bump_seed[0] -= 1; + bump_seed[0] = bump_seed[0].saturating_sub(1); question_mark!(invoke_context.get_compute_meter().consume(cost), result); } *result = Ok(1); @@ -1115,10 +1115,12 @@ impl<'a, 'b> SyscallObject for SyscallSha256<'a, 'b> { compute_budget.mem_op_base_cost.max( compute_budget .sha256_byte_cost - .saturating_mul(val.len() as u64 / 2), + .saturating_mul((val.len() as u64).saturating_div(2)), ) } else { - compute_budget.sha256_byte_cost * (val.len() as u64 / 2) + compute_budget + .sha256_byte_cost + .saturating_mul((val.len() as u64).saturating_div(2)) }; question_mark!(invoke_context.get_compute_meter().consume(cost), result); hasher.hash(bytes); @@ -1136,9 +1138,12 @@ fn get_sysvar( memory_mapping: &MemoryMapping, invoke_context: &mut InvokeContext, ) -> Result> { - invoke_context - .get_compute_meter() - .consume(invoke_context.get_compute_budget().sysvar_base_cost + size_of::() as u64)?; + invoke_context.get_compute_meter().consume( + invoke_context + .get_compute_budget() + .sysvar_base_cost + .saturating_add(size_of::() as u64), + )?; let var = translate_type_mut::(memory_mapping, var_addr, loader_id)?; let sysvar: Arc = sysvar.map_err(SyscallError::InstructionError)?; @@ -1349,10 +1354,12 @@ impl<'a, 'b> SyscallObject for SyscallKeccak256<'a, 'b> { compute_budget.mem_op_base_cost.max( compute_budget .sha256_byte_cost - .saturating_mul(val.len() as u64 / 2), + .saturating_mul((val.len() as u64).saturating_div(2)), ) } else { - compute_budget.sha256_byte_cost * (val.len() as u64 / 2) + compute_budget + .sha256_byte_cost + .saturating_mul((val.len() as u64).saturating_div(2)) }; question_mark!(invoke_context.get_compute_meter().consume(cost), result); hasher.hash(bytes); @@ -1366,8 +1373,8 @@ impl<'a, 'b> SyscallObject for SyscallKeccak256<'a, 'b> { /// This function is incorrect due to arithmetic overflow and only exists for /// backwards compatibility. Instead use program_stubs::is_nonoverlapping. fn check_overlapping_do_not_use(src_addr: u64, dst_addr: u64, n: u64) -> bool { - (src_addr <= dst_addr && src_addr + n > dst_addr) - || (dst_addr <= src_addr && dst_addr + n > src_addr) + (src_addr <= dst_addr && src_addr.saturating_add(n) > dst_addr) + || (dst_addr <= src_addr && dst_addr.saturating_add(n) > src_addr) } fn mem_op_consume<'a, 'b>( @@ -1381,9 +1388,9 @@ fn mem_op_consume<'a, 'b>( { compute_budget .mem_op_base_cost - .max(n / compute_budget.cpi_bytes_per_unit) + .max(n.saturating_div(compute_budget.cpi_bytes_per_unit)) } else { - n / compute_budget.cpi_bytes_per_unit + n.saturating_div(compute_budget.cpi_bytes_per_unit) }; invoke_context.get_compute_meter().consume(cost) } @@ -1417,7 +1424,7 @@ impl<'a, 'b> SyscallObject for SyscallMemcpy<'a, 'b> { if update_syscall_base_costs { let cost = compute_budget .mem_op_base_cost - .max(n / compute_budget.cpi_bytes_per_unit); + .max(n.saturating_div(compute_budget.cpi_bytes_per_unit)); question_mark!(invoke_context.get_compute_meter().consume(cost), result); } @@ -1439,7 +1446,7 @@ impl<'a, 'b> SyscallObject for SyscallMemcpy<'a, 'b> { } if !update_syscall_base_costs { - let cost = n / compute_budget.cpi_bytes_per_unit; + let cost = n.saturating_div(compute_budget.cpi_bytes_per_unit); question_mark!(invoke_context.get_compute_meter().consume(cost), result); }; @@ -1537,11 +1544,11 @@ impl<'a, 'b> SyscallObject for SyscallMemcmp<'a, 'b> { let a = s1[i]; let b = s2[i]; if a != b { - *cmp_result = a as i32 - b as i32; + *cmp_result = (a as i32).saturating_sub(b as i32); *result = Ok(0); return; } - i += 1; + i = i.saturating_add(1); } *cmp_result = 0; *result = Ok(0); @@ -1924,10 +1931,12 @@ impl<'a, 'b> SyscallObject for SyscallBlake3<'a, 'b> { compute_budget.mem_op_base_cost.max( compute_budget .sha256_byte_cost - .saturating_mul(val.len() as u64 / 2), + .saturating_mul((val.len() as u64).saturating_div(2)), ) } else { - compute_budget.sha256_byte_cost * (val.len() as u64 / 2) + compute_budget + .sha256_byte_cost + .saturating_mul((val.len() as u64).saturating_div(2)) }; question_mark!(invoke_context.get_compute_meter().consume(cost), result); hasher.hash(bytes); @@ -2082,7 +2091,8 @@ impl<'a, 'b> SyscallInvokeSigned<'a, 'b> for SyscallInvokeSignedRust<'a, 'b> { )?; invoke_context.get_compute_meter().consume( - data.len() as u64 / invoke_context.get_compute_budget().cpi_bytes_per_unit, + (data.len() as u64) + .saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit), )?; let translated = translate( @@ -2352,7 +2362,9 @@ impl<'a, 'b> SyscallInvokeSigned<'a, 'b> for SyscallInvokeSignedC<'a, 'b> { let vm_data_addr = account_info.data_addr; invoke_context.get_compute_meter().consume( - account_info.data_len / invoke_context.get_compute_budget().cpi_bytes_per_unit, + account_info + .data_len + .saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit), )?; let data = translate_slice_mut::( @@ -2364,7 +2376,7 @@ impl<'a, 'b> SyscallInvokeSigned<'a, 'b> for SyscallInvokeSignedC<'a, 'b> { let first_info_addr = &account_infos[0] as *const _ as u64; let addr = &account_info.data_len as *const u64 as u64; - let vm_addr = account_infos_addr + (addr - first_info_addr); + let vm_addr = account_infos_addr.saturating_add(addr.saturating_sub(first_info_addr)); let _ = translate( memory_mapping, AccessType::Store, @@ -2580,7 +2592,9 @@ fn check_account_infos( len: usize, invoke_context: &mut InvokeContext, ) -> Result<(), EbpfError> { - if len * size_of::() > invoke_context.get_compute_budget().max_cpi_instruction_size { + if len.saturating_mul(size_of::()) + > invoke_context.get_compute_budget().max_cpi_instruction_size + { // Cap the number of account_infos a caller can pass to approximate // maximum that accounts that could be passed in an instruction return Err(SyscallError::TooManyAccounts.into()); @@ -2710,9 +2724,16 @@ fn call<'a, 'b: 'a>( ); } let data_overflow = if do_support_realloc { - new_len > caller_account.original_data_len + MAX_PERMITTED_DATA_INCREASE + new_len + > caller_account + .original_data_len + .saturating_add(MAX_PERMITTED_DATA_INCREASE) } else { - new_len > caller_account.data.len() + MAX_PERMITTED_DATA_INCREASE + new_len + > caller_account + .data + .len() + .saturating_add(MAX_PERMITTED_DATA_INCREASE) }; if data_overflow { ic_msg!( @@ -2770,9 +2791,10 @@ impl<'a, 'b> SyscallObject for SyscallSetReturnData<'a, 'b> { let budget = invoke_context.get_compute_budget(); question_mark!( - invoke_context - .get_compute_meter() - .consume(len / budget.cpi_bytes_per_unit + budget.syscall_base_cost), + invoke_context.get_compute_meter().consume( + len.saturating_div(budget.cpi_bytes_per_unit) + .saturating_add(budget.syscall_base_cost) + ), result ); @@ -2845,9 +2867,10 @@ impl<'a, 'b> SyscallObject for SyscallGetReturnData<'a, 'b> { length = length.min(return_data.len() as u64); if length != 0 { question_mark!( - invoke_context - .get_compute_meter() - .consume((length + size_of::() as u64) / budget.cpi_bytes_per_unit), + invoke_context.get_compute_meter().consume( + (length.saturating_add(size_of::() as u64)) + .saturating_div(budget.cpi_bytes_per_unit) + ), result ); @@ -2994,7 +3017,7 @@ impl<'a, 'b> SyscallObject for SyscallGetProcessedSiblingInstruction<' if index == current_index { return true; } else { - current_index += 1; + current_index = current_index.saturating_add(1); } } false @@ -4190,7 +4213,7 @@ mod tests { MemoryRegion { host_addr: mock_slices.as_ptr() as u64, vm_addr: SEEDS_VA, - len: (seeds.len() * size_of::()) as u64, + len: (seeds.len().saturating_mul(size_of::()) as u64), vm_gap_shift: 63, is_writable: false, }, @@ -4218,7 +4241,7 @@ mod tests { ]; for (i, seed) in seeds.iter().enumerate() { - let vm_addr = SEED_VA + (i as u64 * 0x100000000); + let vm_addr = SEED_VA.saturating_add((i as u64).saturating_mul(0x100000000)); let mock_slice = MockSlice { vm_addr, len: seed.len(),