Skip to content

Commit

Permalink
cpi: direct_mapping: always zero spare capacity if account alloc chan…
Browse files Browse the repository at this point in the history 
…ges (#34141)

If the vector holding an account is reallocated during execution of a callee,
we must zero the spare capacity regardless of whether the account size changed,
because the underlying vector might contain uninitialized memory in the spare
capacity.
  • Loading branch information
@alessandrod
alessandrod authored Nov 20, 2023
1 parent 9a7b681 commit d9147d7
Show file tree
Hide file tree
Showing 2 changed files with 126 additions and 81 deletions.
179 changes: 98 additions & 81 deletions programs/bpf_loader/src/syscalls/cpi.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1302,22 +1302,28 @@ fn update_caller_account(
caller_account.vm_data_addr,
caller_account.original_data_len,
)? {
// Since each instruction account is directly mapped in a memory region
// with a *fixed* length, upon returning from CPI we must ensure that the
// current capacity is at least the original length (what is mapped in
// memory), so that the account's memory region never points to an
// invalid address.
// Since each instruction account is directly mapped in a memory region with a *fixed*
// length, upon returning from CPI we must ensure that the current capacity is at least
// the original length (what is mapped in memory), so that the account's memory region
// never points to an invalid address.
//
// Note that the capacity can be smaller than the original length only if the account is
// reallocated using the AccountSharedData API directly (deprecated). BorrowedAccount
// and CoW don't trigger this, see BorrowedAccount::make_data_mut.
let min_capacity = caller_account.original_data_len;
if callee_account.capacity() < min_capacity {
callee_account
.reserve(min_capacity.saturating_sub(callee_account.get_data().len()))?;
zero_all_mapped_spare_capacity = true;
}

// If an account's data pointer has changed - because of CoW, reserve() as called above
// or because of using AccountSharedData directly (deprecated) - we must update the
// corresponding MemoryRegion in the caller's address space. Address spaces are fixed so
// we don't need to update the MemoryRegion's length.
// If an account's data pointer has changed we must update the corresponding
// MemoryRegion in the caller's address space. Address spaces are fixed so we don't need
// to update the MemoryRegion's length.
//
// An account's data pointer can change if the account is reallocated because of CoW,
// because of BorrowedAccount::make_data_mut or by a program that uses the
// AccountSharedData API directly (deprecated).
let callee_ptr = callee_account.get_data().as_ptr() as u64;
if region.host_addr.get() != callee_ptr {
region.host_addr.set(callee_ptr);
Expand All @@ -1328,7 +1334,6 @@ fn update_caller_account(

let prev_len = *caller_account.ref_to_len_in_vm.get()? as usize;
let post_len = callee_account.get_data().len();
let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len);
if prev_len != post_len {
let max_increase = if direct_mapping && !invoke_context.get_check_aligned() {
0
Expand All @@ -1352,37 +1357,8 @@ fn update_caller_account(
if post_len < prev_len {
if direct_mapping {
// We have two separate regions to zero out: the account data
// and the realloc region.
//
// Here we zero the account data region.
let spare_len = if zero_all_mapped_spare_capacity {
// In the unlikely case where the account data vector has
// changed - which can happen during CoW - we zero the whole
// extra capacity up to the original data length.
//
// The extra capacity up to original data length is
// accessible from the vm and since it's uninitialized
// memory, it could be a source of non determinism.
caller_account.original_data_len
} else {
// If the allocation has not changed, we only zero the
// difference between the previous and current lengths. The
// rest of the memory contains whatever it contained before,
// which is deterministic.
prev_len
}
.saturating_sub(post_len);
if spare_len > 0 {
let dst = callee_account
.spare_data_capacity_mut()?
.get_mut(..spare_len)
.ok_or_else(|| Box::new(InstructionError::AccountDataTooSmall))?
.as_mut_ptr();
// Safety: we check bounds above
unsafe { ptr::write_bytes(dst, 0, spare_len) };
}

// Here we zero the realloc region.
// and the realloc region. Here we zero the realloc region, the
// data region is zeroed further down below.
//
// This is done for compatibility but really only necessary for
// the fringe case of a program calling itself, see
Expand Down Expand Up @@ -1464,50 +1440,91 @@ fn update_caller_account(
)?;
*serialized_len_ptr = post_len as u64;
}
if !direct_mapping {
let to_slice = &mut caller_account.serialized_data;
let from_slice = callee_account
.get_data()
.get(0..post_len)
.ok_or(SyscallError::InvalidLength)?;
if to_slice.len() != from_slice.len() {
return Err(Box::new(InstructionError::AccountDataTooSmall));
}
to_slice.copy_from_slice(from_slice);
} else if realloc_bytes_used > 0 {
// In the is_loader_deprecated case, we must have failed with
// InvalidRealloc by now.
debug_assert!(!is_loader_deprecated);

let to_slice = {
// If a callee reallocs an account, we write into the caller's
// realloc region regardless of whether the caller has write
// permissions to the account or not. If the callee has been able to
// make changes, it means they had permissions to do so, and here
// we're just going to reflect those changes to the caller's frame.

if direct_mapping {
// Here we zero the account data region.
//
// If zero_all_mapped_spare_capacity=true, we need to zero regardless of whether the account
// size changed, because the underlying vector holding the account might have been
// reallocated and contain uninitialized memory in the spare capacity.
//
// See TEST_CPI_CHANGE_ACCOUNT_DATA_MEMORY_ALLOCATION for an example of
// this case.
let spare_len = if zero_all_mapped_spare_capacity {
// In the unlikely case where the account data vector has
// changed - which can happen during CoW - we zero the whole
// extra capacity up to the original data length.
//
// Therefore we temporarily configure the realloc region as writable
// then set it back to whatever state it had.
let realloc_region = caller_account
.realloc_region(memory_mapping, is_loader_deprecated)?
.unwrap(); // unwrapping here is fine, we asserted !is_loader_deprecated
let original_state = realloc_region.state.replace(MemoryState::Writable);
defer! {
realloc_region.state.set(original_state);
};
// The extra capacity up to original data length is
// accessible from the vm and since it's uninitialized
// memory, it could be a source of non determinism.
caller_account.original_data_len
} else {
// If the allocation has not changed, we only zero the
// difference between the previous and current lengths. The
// rest of the memory contains whatever it contained before,
// which is deterministic.
prev_len
}
.saturating_sub(post_len);

if spare_len > 0 {
let dst = callee_account
.spare_data_capacity_mut()?
.get_mut(..spare_len)
.ok_or_else(|| Box::new(InstructionError::AccountDataTooSmall))?
.as_mut_ptr();
// Safety: we check bounds above
unsafe { ptr::write_bytes(dst, 0, spare_len) };
}

translate_slice_mut::<u8>(
memory_mapping,
caller_account
.vm_data_addr
.saturating_add(caller_account.original_data_len as u64),
realloc_bytes_used as u64,
invoke_context.get_check_aligned(),
)?
};
// Propagate changes to the realloc region in the callee up to the caller.
let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len);
if realloc_bytes_used > 0 {
// In the is_loader_deprecated case, we must have failed with
// InvalidRealloc by now.
debug_assert!(!is_loader_deprecated);

let to_slice = {
// If a callee reallocs an account, we write into the caller's
// realloc region regardless of whether the caller has write
// permissions to the account or not. If the callee has been able to
// make changes, it means they had permissions to do so, and here
// we're just going to reflect those changes to the caller's frame.
//
// Therefore we temporarily configure the realloc region as writable
// then set it back to whatever state it had.
let realloc_region = caller_account
.realloc_region(memory_mapping, is_loader_deprecated)?
.unwrap(); // unwrapping here is fine, we asserted !is_loader_deprecated
let original_state = realloc_region.state.replace(MemoryState::Writable);
defer! {
realloc_region.state.set(original_state);
};

translate_slice_mut::<u8>(
memory_mapping,
caller_account
.vm_data_addr
.saturating_add(caller_account.original_data_len as u64),
realloc_bytes_used as u64,
invoke_context.get_check_aligned(),
)?
};
let from_slice = callee_account
.get_data()
.get(caller_account.original_data_len..post_len)
.ok_or(SyscallError::InvalidLength)?;
if to_slice.len() != from_slice.len() {
return Err(Box::new(InstructionError::AccountDataTooSmall));
}
to_slice.copy_from_slice(from_slice);
}
} else {
let to_slice = &mut caller_account.serialized_data;
let from_slice = callee_account
.get_data()
.get(caller_account.original_data_len..post_len)
.get(0..post_len)
.ok_or(SyscallError::InvalidLength)?;
if to_slice.len() != from_slice.len() {
return Err(Box::new(InstructionError::AccountDataTooSmall));
Expand Down
28 changes: 28 additions & 0 deletions programs/sbf/rust/invoke/src/processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1315,6 +1315,34 @@ fn process_instruction(
},
&vec![0; original_data_len - new_len]
);

// Realloc to [0xFC; 2]. Here we keep the same length, but realloc the underlying
// vector. CPI must zero even if the length is unchanged.
invoke(
&create_instruction(
*callee_program_id,
&[
(accounts[ARGUMENT_INDEX].key, true, false),
(callee_program_id, false, false),
],
vec![0xFC; 2],
),
accounts,
)
.unwrap();

// Check that [2..20] is zeroed
let new_len = account.data_len();
assert_eq!(&*account.data.borrow(), &[0xFC; 2]);
assert_eq!(
unsafe {
slice::from_raw_parts(
account.data.borrow().as_ptr().add(new_len),
original_data_len - new_len,
)
},
&vec![0; original_data_len - new_len]
);
}
TEST_WRITE_ACCOUNT => {
msg!("TEST_WRITE_ACCOUNT");
Expand Down

0 comments on commit d9147d7

Please sign in to comment.