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.

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

Ensure that account info address is not in an account #3044

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 15 additions & 6 deletions programs/bpf_loader/src/syscalls/cpi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -321,12 +321,6 @@ impl<'a, 'b> CallerAccount<'a, 'b> {
.saturating_sub(account_info as *const _ as *const u64 as u64);

let ref_to_len_in_vm = if direct_mapping {
// In the same vein as the other check_account_info_pointer() checks, we don't lock this
// pointer to a specific address but we don't want it to be inside accounts, or callees
// might be able to write to the pointed memory.
if data_len_vm_addr >= ebpf::MM_INPUT_START {
return Err(SyscallError::InvalidPointer.into());
}
VmValue::VmAddress {
vm_addr: data_len_vm_addr,
memory_mapping,
Expand Down Expand Up @@ -804,6 +798,21 @@ fn translate_account_infos<'a, T, F>(
where
F: Fn(&T) -> u64,
{
let direct_mapping = invoke_context
.get_feature_set()
.is_active(&feature_set::bpf_account_data_direct_mapping::id());

// In the same vein as the other check_account_info_pointer() checks, we don't lock
// this pointer to a specific address but we don't want it to be inside accounts, or
// callees might be able to write to the pointed memory.
if direct_mapping
&& account_infos_addr
.saturating_add(account_infos_len.saturating_mul(std::mem::size_of::<T>() as u64))
>= ebpf::MM_INPUT_START
{
return Err(SyscallError::InvalidPointer.into());
}

let account_infos = translate_slice::<T>(
memory_mapping,
account_infos_addr,
Expand Down
35 changes: 35 additions & 0 deletions programs/sbf/c/src/invoke/invoke.c
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ static const uint8_t TEST_CPI_INVALID_KEY_POINTER = 35;
static const uint8_t TEST_CPI_INVALID_OWNER_POINTER = 36;
static const uint8_t TEST_CPI_INVALID_LAMPORTS_POINTER = 37;
static const uint8_t TEST_CPI_INVALID_DATA_POINTER = 38;
static const uint8_t TEST_WRITE_ACCOUNT = 40;
static const uint8_t TEST_ACCOUNT_INFO_IN_ACCOUNT = 43;

static const int MINT_INDEX = 0;
static const int ARGUMENT_INDEX = 1;
Expand Down Expand Up @@ -748,6 +750,39 @@ extern uint64_t entrypoint(const uint8_t *input) {
sol_invoke(&instruction, accounts, 4);
break;
}
case TEST_WRITE_ACCOUNT:
{
sol_log("Test TEST_WRITE_ACCOUNT");

uint8_t target_account_index = params.data[1];
uint64_t offset = *((uint64_t*)(params.data + 2));

accounts[target_account_index].data[offset] = params.data[10];
break;
}
case TEST_ACCOUNT_INFO_IN_ACCOUNT:
{
sol_log("Test TEST_ACCOUNT_INFO_IN_ACCOUNT");

uint8_t data[] = { TEST_WRITE_ACCOUNT, 1, 1, 0, 0, 0, 0, 0, 0, 0, 1 };

void *account_info_acc = accounts[1].data + 32;

sol_memcpy(account_info_acc, accounts, sizeof(accounts[0]) * params.ka_num);

SolAccountMeta arguments[] = {
{accounts[INVOKED_PROGRAM_INDEX].key, false, false},
{accounts[ARGUMENT_INDEX].key, true, false},
{accounts[MINT_INDEX].key, false, false},
};

const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key,
arguments, SOL_ARRAY_SIZE(arguments),
data, SOL_ARRAY_SIZE(data)};

sol_invoke(&instruction, account_info_acc, params.ka_num);
break;
}

default:
sol_panic();
Expand Down
38 changes: 38 additions & 0 deletions programs/sbf/rust/invoke/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1484,6 +1484,44 @@ fn process_instruction<'a>(
)
.unwrap();
}
TEST_ACCOUNT_INFO_IN_ACCOUNT => {
msg!("TEST_ACCOUNT_INFO_IN_ACCOUNT");

let account_offset = usize::from_le_bytes(instruction_data[1..9].try_into().unwrap());

let mut instruction_data = vec![TEST_WRITE_ACCOUNT, 1];
instruction_data.extend_from_slice(&1u64.to_le_bytes());
instruction_data.push(1);

let data = accounts[ARGUMENT_INDEX].data.borrow().as_ptr();
let len = accounts.len();

let account_info_in_account: &mut [AccountInfo] = unsafe {
std::slice::from_raw_parts_mut(data.add(account_offset) as *mut AccountInfo, len)
};

unsafe {
std::ptr::copy_nonoverlapping(
accounts.as_ptr(),
account_info_in_account.as_mut_ptr(),
len,
);
}

invoke(
&create_instruction(
*program_id,
&[
(program_id, false, false),
(accounts[1].key, true, false),
(accounts[0].key, false, false),
],
instruction_data.to_vec(),
),
account_info_in_account,
)
.unwrap();
}
_ => panic!("unexpected program data"),
}

Expand Down
1 change: 1 addition & 0 deletions programs/sbf/rust/invoke_dep/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ pub const TEST_CPI_CHANGE_ACCOUNT_DATA_MEMORY_ALLOCATION: u8 = 39;
pub const TEST_WRITE_ACCOUNT: u8 = 40;
pub const TEST_CALLEE_ACCOUNT_UPDATES: u8 = 41;
pub const TEST_STACK_HEAP_ZEROED: u8 = 42;
pub const TEST_ACCOUNT_INFO_IN_ACCOUNT: u8 = 43;

pub const MINT_INDEX: usize = 0;
pub const ARGUMENT_INDEX: usize = 1;
Expand Down
72 changes: 72 additions & 0 deletions programs/sbf/tests/programs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4929,6 +4929,78 @@ fn test_update_callee_account() {
}
}

#[test]
fn test_account_info_in_account() {
solana_logger::setup();

let GenesisConfigInfo {
genesis_config,
mint_keypair,
..
} = create_genesis_config(100_123_456_789);

let mut programs = Vec::new();
#[cfg(feature = "sbf_c")]
{
programs.push("invoke");
}
#[cfg(feature = "sbf_rust")]
{
programs.push("solana_sbf_rust_invoke");
}

for program in programs {
for direct_mapping in [false, true] {
let mut bank = Bank::new_for_tests(&genesis_config);
let feature_set = Arc::make_mut(&mut bank.feature_set);
// by default test banks have all features enabled, so we only need to
// disable when needed
if !direct_mapping {
feature_set.deactivate(&feature_set::bpf_account_data_direct_mapping::id());
}

let (bank, bank_forks) = bank.wrap_with_bank_forks_for_tests();
let mut bank_client = BankClient::new_shared(bank.clone());
let authority_keypair = Keypair::new();

let (bank, invoke_program_id) = load_upgradeable_program_and_advance_slot(
&mut bank_client,
bank_forks.as_ref(),
&mint_keypair,
&authority_keypair,
program,
);

let account_keypair = Keypair::new();

let mint_pubkey = mint_keypair.pubkey();

let account_metas = vec![
AccountMeta::new(mint_pubkey, true),
AccountMeta::new(account_keypair.pubkey(), false),
AccountMeta::new_readonly(invoke_program_id, false),
];

let mut instruction_data = vec![TEST_ACCOUNT_INFO_IN_ACCOUNT];
instruction_data.extend_from_slice(32usize.to_le_bytes().as_ref());

let instruction =
Instruction::new_with_bytes(invoke_program_id, &instruction_data, account_metas);

let account = AccountSharedData::new(42, 10240, &invoke_program_id);

bank.store_account(&account_keypair.pubkey(), &account);

let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction);
if direct_mapping {
assert!(result.is_err());
} else {
assert!(result.is_ok());
}
}
}
}

#[test]
fn test_clone_account_data() {
// Test cloning account data works as expect with
Expand Down
Loading