Skip to content

Commit

Permalink
bpf_loader: direct_mapping: enforce *all* account info pointers to be…
Browse files Browse the repository at this point in the history 
… immutable
  • Loading branch information
@alessandrod
alessandrod committed Jun 20, 2023
1 parent be84d74 commit e03f6cd
Show file tree
Hide file tree
Showing 7 changed files with 276 additions and 66 deletions.
3 changes: 3 additions & 0 deletions program-runtime/src/invoke_context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ pub struct SyscallContext {
pub struct SerializedAccountMetadata {
pub original_data_len: usize,
pub vm_data_addr: u64,
pub vm_key_addr: u64,
pub vm_lamports_addr: u64,
pub vm_owner_addr: u64,
}

pub struct InvokeContext<'a> {
Expand Down
42 changes: 30 additions & 12 deletions programs/bpf_loader/src/serialization.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,12 @@ impl Serializer {
self.buffer.fill_write(num, value)
}

pub fn write<T: Pod>(&mut self, value: T) {
pub fn write<T: Pod>(&mut self, value: T) -> u64 {
self.debug_assert_alignment::<T>();
let vaddr = self
.vaddr
.saturating_add(self.buffer.len() as u64)
.saturating_sub(self.region_start as u64);
// Safety:
// in serialize_parameters_(aligned|unaligned) first we compute the
// required size then we write into the newly allocated buffer. There's
Expand All @@ -68,14 +72,22 @@ impl Serializer {
unsafe {
self.buffer.write_unchecked(value);
}

vaddr
}

fn write_all(&mut self, value: &[u8]) {
fn write_all(&mut self, value: &[u8]) -> u64 {
let vaddr = self
.vaddr
.saturating_add(self.buffer.len() as u64)
.saturating_sub(self.region_start as u64);
// Safety:
// see write() - the buffer is guaranteed to be large enough
unsafe {
self.buffer.write_all_unchecked(value);
}

vaddr
}

fn write_account(
Expand Down Expand Up @@ -333,15 +345,18 @@ fn serialize_parameters_unaligned(
s.write::<u8>(NON_DUP_MARKER);
s.write::<u8>(account.is_signer() as u8);
s.write::<u8>(account.is_writable() as u8);
s.write_all(account.get_key().as_ref());
s.write::<u64>(account.get_lamports().to_le());
let vm_key_addr = s.write_all(account.get_key().as_ref());
let vm_lamports_addr = s.write::<u64>(account.get_lamports().to_le());
s.write::<u64>((account.get_data().len() as u64).to_le());
let vm_data_addr = s.write_account(&mut account)?;
s.write_all(account.get_owner().as_ref());
let vm_owner_addr = s.write_all(account.get_owner().as_ref());
s.write::<u8>(account.is_executable() as u8);
s.write::<u64>((account.get_rent_epoch()).to_le());
accounts_metadata.push(SerializedAccountMetadata {
original_data_len: account.get_data().len(),
vm_key_addr,
vm_lamports_addr,
vm_owner_addr,
vm_data_addr,
});
}
Expand Down Expand Up @@ -422,7 +437,7 @@ fn serialize_parameters_aligned(
),
InstructionError,
> {
let mut account_lengths = Vec::with_capacity(accounts.len());
let mut accounts_metadata = Vec::with_capacity(accounts.len());
// Calculate size in order to alloc once
let mut size = size_of::<u64>();
for account in &accounts {
Expand Down Expand Up @@ -465,19 +480,22 @@ fn serialize_parameters_aligned(
s.write::<u8>(borrowed_account.is_writable() as u8);
s.write::<u8>(borrowed_account.is_executable() as u8);
s.write_all(&[0u8, 0, 0, 0]);
s.write_all(borrowed_account.get_key().as_ref());
s.write_all(borrowed_account.get_owner().as_ref());
s.write::<u64>(borrowed_account.get_lamports().to_le());
let vm_key_addr = s.write_all(borrowed_account.get_key().as_ref());
let vm_owner_addr = s.write_all(borrowed_account.get_owner().as_ref());
let vm_lamports_addr = s.write::<u64>(borrowed_account.get_lamports().to_le());
s.write::<u64>((borrowed_account.get_data().len() as u64).to_le());
let vm_data_addr = s.write_account(&mut borrowed_account)?;
s.write::<u64>((borrowed_account.get_rent_epoch()).to_le());
account_lengths.push(SerializedAccountMetadata {
accounts_metadata.push(SerializedAccountMetadata {
original_data_len: borrowed_account.get_data().len(),
vm_key_addr,
vm_owner_addr,
vm_lamports_addr,
vm_data_addr,
});
}
SerializeAccount::Duplicate(position) => {
account_lengths.push(account_lengths.get(position as usize).unwrap().clone());
accounts_metadata.push(accounts_metadata.get(position as usize).unwrap().clone());
s.write::<u8>(position as u8);
s.write_all(&[0u8, 0, 0, 0, 0, 0, 0]);
}
Expand All @@ -488,7 +506,7 @@ fn serialize_parameters_aligned(
s.write_all(program_id.as_ref());

let (mem, regions) = s.finish();
Ok((mem, regions, account_lengths))
Ok((mem, regions, accounts_metadata))
}

pub fn deserialize_parameters_aligned<I: IntoIterator<Item = usize>>(
Expand Down
138 changes: 87 additions & 51 deletions programs/bpf_loader/src/syscalls/cpi.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,20 @@ use {
std::{mem, ptr},
};

macro_rules! check_account_info_pointer {
($ptr:expr, $expected:expr, $log:literal) => {
if $ptr != $expected {
log::debug!(
"Invalid account info pointer `{}': {:#x} != {:#x}",
$log,
$ptr,
$expected
);
return Err(SyscallError::InvalidPointer.into());
}
};
}

/// Host side representation of AccountInfo or SolAccountInfo passed to the CPI syscall.
///
/// At the start of a CPI, this can be different from the data stored in the
Expand Down Expand Up @@ -52,6 +66,23 @@ impl<'a> CallerAccount<'a> {
account_info: &AccountInfo,
account_metadata: &SerializedAccountMetadata,
) -> Result<CallerAccount<'a>, Error> {
let direct_mapping = invoke_context
.feature_set
.is_active(&feature_set::bpf_account_data_direct_mapping::id());

if direct_mapping {
check_account_info_pointer!(
account_info.key as *const _ as u64,
account_metadata.vm_key_addr,
"key"
);
check_account_info_pointer!(
account_info.owner as *const _ as u64,
account_metadata.vm_owner_addr,
"owner"
);
}

let original_data_len = account_metadata.original_data_len;
// account_info points to host memory. The addresses used internally are
// in vm space so they need to be translated.
Expand All @@ -62,36 +93,31 @@ impl<'a> CallerAccount<'a> {
account_info.lamports.as_ptr() as u64,
invoke_context.get_check_aligned(),
)?;
if direct_mapping {
check_account_info_pointer!(*ptr, account_metadata.vm_lamports_addr, "lamports");
}
translate_type_mut::<u64>(memory_mapping, *ptr, invoke_context.get_check_aligned())?
};

let owner = translate_type_mut::<Pubkey>(
memory_mapping,
account_info.owner as *const _ as u64,
invoke_context.get_check_aligned(),
)?;

let (serialized_data, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr) = {
let direct_mapping = invoke_context
.feature_set
.is_active(&feature_set::bpf_account_data_direct_mapping::id());

// Double translate data out of RefCell
let data = *translate_type::<&[u8]>(
memory_mapping,
account_info.data.as_ptr() as *const _ as u64,
invoke_context.get_check_aligned(),
)?;

if direct_mapping && data.as_ptr() as u64 != account_metadata.vm_data_addr {
// When direct mapping is enabled, we might need to update the
// account data memory region across CPI calls (see
// update_caller_account). Since the only way we have to
// retrieve the region is based on its vm address, we enforce vm
// addresses to be stable.
//
// An account can still be mutated and resized of course, but it must be
// done in place.
return Err(SyscallError::AccountDataPointerChanged.into());
if direct_mapping {
check_account_info_pointer!(
data.as_ptr() as u64,
account_metadata.vm_data_addr,
"data"
);
}

consume_compute_meter(
Expand Down Expand Up @@ -176,10 +202,35 @@ impl<'a> CallerAccount<'a> {
account_info: &SolAccountInfo,
account_metadata: &SerializedAccountMetadata,
) -> Result<CallerAccount<'a>, Error> {
let direct_mapping = invoke_context
.feature_set
.is_active(&feature_set::bpf_account_data_direct_mapping::id());

if direct_mapping {
check_account_info_pointer!(account_info.key_addr, account_metadata.vm_key_addr, "key");

check_account_info_pointer!(
account_info.owner_addr,
account_metadata.vm_owner_addr,
"owner"
);

check_account_info_pointer!(
account_info.lamports_addr,
account_metadata.vm_lamports_addr,
"lamports"
);

check_account_info_pointer!(
account_info.data_addr,
account_metadata.vm_data_addr,
"data"
);
}

let original_data_len = account_metadata.original_data_len;
// account_info points to host memory. The addresses used internally are
// in vm space so they need to be translated.

let lamports = translate_type_mut::<u64>(
memory_mapping,
account_info.lamports_addr,
Expand All @@ -191,22 +242,6 @@ impl<'a> CallerAccount<'a> {
invoke_context.get_check_aligned(),
)?;

let direct_mapping = invoke_context
.feature_set
.is_active(&feature_set::bpf_account_data_direct_mapping::id());

let vm_data_addr = account_info.data_addr;
if direct_mapping && vm_data_addr != account_metadata.vm_data_addr {
// When direct mapping is enabled, we might need to update the
// account data memory region across CPI calls (see
// update_caller_account). Since the only way we have to
// retrieve the region is based on its vm address, we enforce vm
// addresses to be stable.
//
// An account can still be mutated and resized of course, but it must be
// done in place.
return Err(SyscallError::AccountDataPointerChanged.into());
}
consume_compute_meter(
invoke_context,
account_info
Expand All @@ -220,7 +255,7 @@ impl<'a> CallerAccount<'a> {
} else {
translate_slice_mut::<u8>(
memory_mapping,
vm_data_addr,
account_info.data_addr,
account_info.data_len,
invoke_context.get_check_aligned(),
invoke_context.get_check_size(),
Expand Down Expand Up @@ -262,7 +297,7 @@ impl<'a> CallerAccount<'a> {
owner,
original_data_len,
serialized_data,
vm_data_addr,
vm_data_addr: account_info.data_addr,
ref_to_len_in_vm,
serialized_len_ptr,
executable: account_info.executable,
Expand Down Expand Up @@ -1597,7 +1632,8 @@ mod tests {

let key = Pubkey::new_unique();
let vm_addr = MM_INPUT_START;
let (_mem, region, vm_data_addr) = MockAccountInfo::new(key, &account).into_region(vm_addr);
let (_mem, region, account_metadata) =
MockAccountInfo::new(key, &account).into_region(vm_addr);

let config = Config {
aligned_memory_mapping: false,
Expand All @@ -1612,10 +1648,7 @@ mod tests {
&memory_mapping,
vm_addr,
account_info,
&SerializedAccountMetadata {
original_data_len: account.data().len(),
vm_data_addr,
},
&account_metadata,
)
.unwrap();
assert_eq!(*caller_account.lamports, account.lamports());
Expand Down Expand Up @@ -2346,7 +2379,8 @@ mod tests {
let original_data_len = account.data().len();

let vm_addr = MM_INPUT_START;
let (_mem, region, vm_data_addr) = MockAccountInfo::new(key, &account).into_region(vm_addr);
let (_mem, region, account_metadata) =
MockAccountInfo::new(key, &account).into_region(vm_addr);

let config = Config {
aligned_memory_mapping: false,
Expand All @@ -2363,15 +2397,7 @@ mod tests {
&[1, 1]
);

mock_create_vm!(
_vm,
Vec::new(),
vec![SerializedAccountMetadata {
original_data_len,
vm_data_addr
}],
&mut invoke_context
);
mock_create_vm!(_vm, Vec::new(), vec![account_metadata], &mut invoke_context);

let accounts = SyscallInvokeSignedRust::translate_accounts(
&[
Expand Down Expand Up @@ -2674,7 +2700,7 @@ mod tests {
}
}

fn into_region(self, vm_addr: u64) -> (Vec<u8>, MemoryRegion, u64) {
fn into_region(self, vm_addr: u64) -> (Vec<u8>, MemoryRegion, SerializedAccountMetadata) {
let size = mem::size_of::<AccountInfo>()
+ mem::size_of::<Pubkey>() * 2
+ mem::size_of::<RcBox<RefCell<&mut u64>>>()
Expand Down Expand Up @@ -2735,7 +2761,17 @@ mod tests {
}

let region = MemoryRegion::new_writable(data.as_mut_slice(), vm_addr as u64);
(data, region, data_addr as u64)
(
data,
region,
SerializedAccountMetadata {
original_data_len: self.data.len(),
vm_key_addr: key_addr as u64,
vm_lamports_addr: lamports_addr as u64,
vm_owner_addr: owner_addr as u64,
vm_data_addr: data_addr as u64,
},
)
}
}

Expand Down
4 changes: 2 additions & 2 deletions programs/bpf_loader/src/syscalls/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ pub enum SyscallError {
},
#[error("InvalidAttribute")]
InvalidAttribute,
#[error("Account data pointer changed")]
AccountDataPointerChanged,
#[error("Invalid pointer")]
InvalidPointer,
}

type Error = Box<dyn std::error::Error>;
Expand Down
4 changes: 4 additions & 0 deletions programs/sbf/rust/invoke/src/instructions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ pub const TEST_CPI_ACCOUNT_UPDATE_CALLEE_GROWS: u8 = 28;
pub const TEST_CPI_ACCOUNT_UPDATE_CALLEE_SHRINKS_SMALLER_THAN_ORIGINAL_LEN: u8 = 29;
pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS: u8 = 30;
pub const TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS_NESTED: u8 = 31;
pub const TEST_CPI_INVALID_KEY_POINTER: u8 = 32;
pub const TEST_CPI_INVALID_OWNER_POINTER: u8 = 33;
pub const TEST_CPI_INVALID_LAMPORTS_POINTER: u8 = 34;
pub const TEST_CPI_INVALID_DATA_POINTER: u8 = 35;

pub const MINT_INDEX: usize = 0;
pub const ARGUMENT_INDEX: usize = 1;
Expand Down
Loading

0 comments on commit e03f6cd

Please sign in to comment.