Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

EVM: dont panic on invalid precompile address #1042

Merged
merged 4 commits into from
Jan 13, 2023
Merged

EVM: dont panic on invalid precompile address #1042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ac292a9
use full U256 for precompile check methods
mriise Jan 13, 2023
5c973e8
return nothing on invalid precompile address
mriise Jan 13, 2023
823e2ac
add comment about precompile addresses in get_contract_type
mriise Jan 13, 2023
6b4a528
use Option<Result> to catch non-existing precompile, use EthAddress i…
mriise Jan 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion actors/evm/src/interpreter/address.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ impl TryFrom<EthAddress> for Address {
impl TryFrom<&EthAddress> for Address {
type Error = StatusCode;
fn try_from(addr: &EthAddress) -> Result<Self, Self::Error> {
if is_reserved_precompile_address(addr.0) {
if is_reserved_precompile_address(addr) {
return Err(StatusCode::BadAddress(format!(
"Cannot convert a precompile address: {:?} to an f4 address",
addr
Expand Down
41 changes: 26 additions & 15 deletions actors/evm/src/interpreter/instructions/call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use fvm_ipld_encoding::ipld_block::IpldBlock;
use fvm_ipld_encoding::BytesDe;
use fvm_shared::{address::Address, sys::SendFlags, IPLD_RAW, METHOD_SEND};

use crate::interpreter::precompiles::PrecompileContext;
use crate::interpreter::precompiles::{is_reserved_precompile_address, PrecompileContext};

use super::ext::{get_contract_type, get_evm_bytecode_cid, ContractType};

Expand Down Expand Up @@ -196,29 +196,40 @@ pub fn call_generic<RT: Runtime>(
&[]
};

if precompiles::Precompiles::<RT>::is_precompile(&dst) {
if is_reserved_precompile_address(&dst.into()) {
let context =
PrecompileContext { call_type: kind, gas_limit: effective_gas_limit(system, gas) };
if log::log_enabled!(log::Level::Info) {
// log input to the precompile, but make sure we dont log _too_ much.
let mut input_hex = hex::encode(input_data);
input_hex.truncate(512);
log::info!(target: "evm", "Calling Precompile:\n\taddress: {:x?}\n\tcontext: {:?}\n\tinput: {}", EthAddress::try_from(dst).unwrap_or(EthAddress([0xff; 20])), context, input_hex);
}

match precompiles::Precompiles::call_precompile(system, dst, input_data, context) {
Ok(return_data) => (1, return_data),
Err(err) => {
log::error!(target: "evm", "Precompile failed: error {:?}", err);
// precompile failed, exit with reverted and no output
Some(res) => {
if log::log_enabled!(log::Level::Info) {
// log input to the precompile, but make sure we dont log _too_ much.
let mut input_hex = hex::encode(input_data);
input_hex.truncate(512);
log::info!(target: "evm", "Call Precompile:\n\taddress: {:x?}\n\tcontext: {:?}\n\tinput: {}", EthAddress::try_from(dst).unwrap_or(EthAddress([0xff; 20])), context, input_hex);
}

match res {
Ok(return_data) => (1, return_data),
Err(err) => {
log::error!(target: "evm", "Precompile failed: error {:?}", err);
// precompile failed, exit with reverted and no output
(0, vec![])
}
}
}
None => {
log::warn!(target: "evm", "Non-existing precompile address: {:?}", EthAddress::from(dst));
(0, vec![])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this should actually return success.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er, well... You know what? I don't care. Technically, we should return success.... but this is just so stupid.

Let's just return an error.

}
}
} else {
let call_result = match kind {
CallKind::Call | CallKind::StaticCall => {
let dst_addr: EthAddress = dst.into();
let dst_addr: Address = dst_addr.try_into().expect("address is a precompile");
let dst_addr: Address = dst_addr.try_into().map_err(|_| ActorError::assertion_failed(
"Reached a precompile address when a precompile should've been caught earlier in the system"
.to_string(),
))?;

// Special casing for account/placeholder/non-existent actors: we just do a SEND (method 0)
// which allows us to transfer funds (and create placeholders)
Expand Down Expand Up @@ -307,7 +318,7 @@ pub fn call_generic<RT: Runtime>(
Err(ActorError::forbidden("cannot delegate-call to native actors".into()))
}
ContractType::Precompile => Err(ActorError::assertion_failed(
"Reached a precompile address when a precompile should've been caught earlier in the system"
"Reached a precompile address in DelegateCall when a precompile should've been caught earlier in the system"
.to_string(),
)),
},
Expand Down
3 changes: 2 additions & 1 deletion actors/evm/src/interpreter/instructions/ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ pub enum ContractType {
pub fn get_contract_type<RT: Runtime>(rt: &RT, addr: U256) -> ContractType {
let addr: EthAddress = addr.into();
// precompiles cant be resolved by the FVM
if Precompiles::<RT>::is_precompile(&addr.as_evm_word()) {
// addresses passed in precompile range will be returned as NotFound; EAM asserts that no actors can be deployed in the precompile reserved range
if Precompiles::<RT>::is_precompile(addr.as_evm_word()) {
return ContractType::Precompile;
}

Expand Down
64 changes: 31 additions & 33 deletions actors/evm/src/interpreter/precompiles/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::marker::PhantomData;
use fil_actors_runtime::runtime::Runtime;
use substrate_bn::{CurveError, GroupError};

use super::{instructions::call::CallKind, System, U256};
use super::{address::EthAddress, instructions::call::CallKind, System, U256};

mod evm;
mod fvm;
Expand Down Expand Up @@ -61,8 +61,8 @@ const fn gen_native_precompiles<RT: Runtime>() -> [PrecompileFn<RT>; 4] {
}
}

pub fn is_reserved_precompile_address(addr: [u8; 20]) -> bool {
let [prefix, middle @ .., index] = addr;
pub fn is_reserved_precompile_address(addr: &EthAddress) -> bool {
let [prefix, middle @ .., index] = addr.0;
(prefix == 0x00 || prefix == NATIVE_PRECOMPILE_ADDRESS_PREFIX)
&& middle == [0u8; 18]
&& index > 0
Expand All @@ -74,11 +74,10 @@ impl<RT: Runtime> Precompiles<RT> {
const EVM_PRECOMPILES: [PrecompileFn<RT>; 9] = gen_evm_precompiles();
const NATIVE_PRECOMPILES: [PrecompileFn<RT>; 4] = gen_native_precompiles();

fn lookup_precompile(addr: &[u8; 32]) -> Option<PrecompileFn<RT>> {
// unrwap will never panic, 32 - 12 = 20
let addr: [u8; 20] = addr[12..].try_into().unwrap();
let [prefix, _m @ .., index] = addr;
if is_reserved_precompile_address(addr) {
fn lookup_precompile(addr: U256) -> Option<PrecompileFn<RT>> {
let addr: EthAddress = addr.into();
let [prefix, _m @ .., index] = addr.0;
if is_reserved_precompile_address(&addr) {
let index = index as usize - 1;
match prefix {
NATIVE_PRECOMPILE_ADDRESS_PREFIX => Self::NATIVE_PRECOMPILES.get(index),
Expand All @@ -92,22 +91,21 @@ impl<RT: Runtime> Precompiles<RT> {
}

/// Precompile Context will be flattened to None if not calling the call_actor precompile.
/// Panics if address is not a precompile.
/// Returns `None` if precompile does not exist at the address provided.
pub fn call_precompile(
system: &mut System<RT>,
precompile_addr: U256,
input: &[u8],
context: PrecompileContext,
) -> PrecompileResult {
unsafe {
Self::lookup_precompile(&precompile_addr.to_bytes()).unwrap()(system, input, context)
}
) -> Option<PrecompileResult> {
Self::lookup_precompile(precompile_addr)
.map(|precompile_fn| unsafe { precompile_fn(system, input, context) })
}

/// Checks if word represents
/// Checks if word is an existing precompile
#[inline]
pub fn is_precompile(addr: &U256) -> bool {
!addr.is_zero() && Self::lookup_precompile(&addr.to_bytes()).is_some()
pub fn is_precompile(addr: U256) -> bool {
!addr.is_zero() && Self::lookup_precompile(addr).is_some()
}
}

Expand All @@ -123,7 +121,7 @@ pub enum PrecompileError {
CallForbidden,
}

#[derive(Debug, PartialEq, Eq)]
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub struct PrecompileContext {
pub call_type: CallKind,
pub gas_limit: u64,
Expand Down Expand Up @@ -160,49 +158,49 @@ mod test {
#[test]
fn is_native_precompile() {
let addr = EthAddress(hex_literal::hex!("fe00000000000000000000000000000000000001"));
assert!(Precompiles::<MockRuntime>::is_precompile(&addr.as_evm_word()));
assert!(is_reserved_precompile_address(addr.0));
assert!(Precompiles::<MockRuntime>::is_precompile(addr.as_evm_word()));
assert!(is_reserved_precompile_address(&addr));
}

#[test]
fn is_evm_precompile() {
let addr = EthAddress(hex_literal::hex!("0000000000000000000000000000000000000001"));
assert!(Precompiles::<MockRuntime>::is_precompile(&addr.as_evm_word()));
assert!(is_reserved_precompile_address(addr.0));
assert!(Precompiles::<MockRuntime>::is_precompile(addr.as_evm_word()));
assert!(is_reserved_precompile_address(&addr));
}

#[test]
fn is_over_precompile() {
let addr = EthAddress(hex_literal::hex!("ff00000000000000000000000000000000000001"));
assert!(!Precompiles::<MockRuntime>::is_precompile(&addr.as_evm_word()));
assert!(!is_reserved_precompile_address(addr.0));
assert!(!Precompiles::<MockRuntime>::is_precompile(addr.as_evm_word()));
assert!(!is_reserved_precompile_address(&addr));
}

#[test]
fn zero_addr_precompile() {
let eth_addr = EthAddress(hex_literal::hex!("fe00000000000000000000000000000000000000"));
let native_addr = EthAddress(hex_literal::hex!("0000000000000000000000000000000000000000"));
assert!(!Precompiles::<MockRuntime>::is_precompile(&eth_addr.as_evm_word()));
assert!(!Precompiles::<MockRuntime>::is_precompile(&native_addr.as_evm_word()));
assert!(!is_reserved_precompile_address(eth_addr.0));
assert!(!is_reserved_precompile_address(native_addr.0));
assert!(!Precompiles::<MockRuntime>::is_precompile(eth_addr.as_evm_word()));
assert!(!Precompiles::<MockRuntime>::is_precompile(native_addr.as_evm_word()));
assert!(!is_reserved_precompile_address(&eth_addr));
assert!(!is_reserved_precompile_address(&native_addr));
}

#[test]
fn between_precompile() {
let addr = EthAddress(hex_literal::hex!("a000000000000000000000000000000000000001"));
assert!(!Precompiles::<MockRuntime>::is_precompile(&addr.as_evm_word()));
assert!(!is_reserved_precompile_address(addr.0));
assert!(!Precompiles::<MockRuntime>::is_precompile(addr.as_evm_word()));
assert!(!is_reserved_precompile_address(&addr));
}

#[test]
fn bad_index() {
let eth_addr = EthAddress(hex_literal::hex!("fe00000000000000000000000000000000000020"));
let native_addr = EthAddress(hex_literal::hex!("0000000000000000000000000000000000000020"));
assert!(!Precompiles::<MockRuntime>::is_precompile(&eth_addr.as_evm_word()));
assert!(!Precompiles::<MockRuntime>::is_precompile(&native_addr.as_evm_word()));
assert!(!Precompiles::<MockRuntime>::is_precompile(eth_addr.as_evm_word()));
assert!(!Precompiles::<MockRuntime>::is_precompile(native_addr.as_evm_word()));
// reserved doesn't check index is within range
assert!(is_reserved_precompile_address(eth_addr.0));
assert!(is_reserved_precompile_address(native_addr.0));
assert!(is_reserved_precompile_address(&eth_addr));
assert!(is_reserved_precompile_address(&native_addr));
}
}