Skip to content

Commit

Permalink
bpf_loader: direct_mapping: return ExternalAccountDataModified
Browse files Browse the repository at this point in the history 
Map access violation errors to ExternalAccountDataModified instead of
ReadonlyDataModified when modifying an account that is writable but not
owned by the current program. Increases compatibility with pre-direct
mapping.
  • Loading branch information
@alessandrod
alessandrod committed Jun 20, 2023
1 parent e03f6cd commit fa28958
Show file tree
Hide file tree
Showing 5 changed files with 165 additions and 102 deletions.
1 change: 1 addition & 0 deletions program-runtime/src/invoke_context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ pub struct SyscallContext {
#[derive(Debug, Clone)]
pub struct SerializedAccountMetadata {
pub original_data_len: usize,
pub is_writable: bool,
pub vm_data_addr: u64,
pub vm_key_addr: u64,
pub vm_lamports_addr: u64,
Expand Down
49 changes: 40 additions & 9 deletions programs/bpf_loader/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1547,7 +1547,14 @@ fn execute<'a, 'b: 'a>(
let log_collector = invoke_context.get_log_collector();
let transaction_context = &invoke_context.transaction_context;
let instruction_context = transaction_context.get_current_instruction_context()?;
let program_id = *instruction_context.get_last_program_key(transaction_context)?;
let (program_id, is_loader_deprecated) = {
let program_account =
instruction_context.try_borrow_last_program_account(transaction_context)?;
(
*program_account.get_key(),
*program_account.get_owner() == bpf_loader_deprecated::id(),
)
};
#[cfg(any(target_os = "windows", not(target_arch = "x86_64")))]
let use_jit = false;
#[cfg(all(not(target_os = "windows"), target_arch = "x86_64"))]
Expand All @@ -1570,11 +1577,23 @@ fn execute<'a, 'b: 'a>(
// save the account addresses so in case of AccessViolation below we can
// map to InstructionError::ReadonlyDataModified, which is easier to
// diagnose from developers
let account_region_addrs = regions
let account_region_addrs = accounts_metadata
.iter()
.map(|r| r.vm_addr..r.vm_addr.saturating_add(r.len))
.map(|m| {
let mut vm_end = m.vm_data_addr.saturating_add(m.original_data_len as u64);
if !is_loader_deprecated {
vm_end = vm_end.saturating_add(MAX_PERMITTED_DATA_INCREASE as u64)
}
(m.vm_data_addr..vm_end, m.is_writable)
})
.collect::<Vec<_>>();
let addr_is_account_data = |addr: u64| account_region_addrs.iter().any(|r| r.contains(&addr));
let addr_is_account_data = |addr: u64| {
account_region_addrs
.iter()
.find(|(r, _)| r.contains(&addr))
.map(|(_, is_writable)| (true, *is_writable))
.unwrap_or((false, false))
};

let mut create_vm_time = Measure::start("create_vm");
let mut execute_time;
Expand Down Expand Up @@ -1646,11 +1665,23 @@ fn execute<'a, 'b: 'a>(
address,
_size,
_section_name,
)) if addr_is_account_data(*address) => {
// We can get here if direct_mapping is enabled and a program tries to
// write to a readonly account. Map the error to ReadonlyDataModified so
// it's easier for devs to diagnose what happened.
Box::new(InstructionError::ReadonlyDataModified)
)) => {
let (is_account_data, is_writable) = addr_is_account_data(*address);
if is_account_data {
// We can get here if direct_mapping is enabled and
// a program tries to write to a readonly region.
// Map the error to ReadonlyDataModified if the
// account !is_writable, and to
// ExternalAccountDataModified if the executing
// program doesn't own the account.
Box::new(if is_writable {
InstructionError::ExternalAccountDataModified
} else {
InstructionError::ReadonlyDataModified
})
} else {
error
}
}
_ => error,
};
Expand Down
2 changes: 2 additions & 0 deletions programs/bpf_loader/src/serialization.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ fn serialize_parameters_unaligned(
vm_lamports_addr,
vm_owner_addr,
vm_data_addr,
is_writable: account.is_writable(),
});
}
};
Expand Down Expand Up @@ -492,6 +493,7 @@ fn serialize_parameters_aligned(
vm_owner_addr,
vm_lamports_addr,
vm_data_addr,
is_writable: borrowed_account.is_writable(),
});
}
SerializeAccount::Duplicate(position) => {
Expand Down
1 change: 1 addition & 0 deletions programs/bpf_loader/src/syscalls/cpi.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2770,6 +2770,7 @@ mod tests {
vm_lamports_addr: lamports_addr as u64,
vm_owner_addr: owner_addr as u64,
vm_data_addr: data_addr as u64,
is_writable: self.is_writable,
},
)
}
Expand Down
214 changes: 121 additions & 93 deletions programs/sbf/tests/programs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3939,107 +3939,135 @@ fn test_program_sbf_inner_instruction_alignment_checks() {
fn test_cpi_account_ownership_writability() {
solana_logger::setup();

let GenesisConfigInfo {
genesis_config,
mint_keypair,
..
} = create_genesis_config(100_123_456_789);
let mut bank = Bank::new_for_tests(&genesis_config);
bank.feature_set = Arc::new(FeatureSet::all_enabled());
let bank = Arc::new(bank);
let mut bank_client = BankClient::new_shared(&bank);

let invoke_program_id = load_program(
&bank_client,
&bpf_loader::id(),
&mint_keypair,
"solana_sbf_rust_invoke",
);

let (bank, invoked_program_id) = load_program_and_advance_slot(
&mut bank_client,
&bpf_loader::id(),
&mint_keypair,
"solana_sbf_rust_invoked",
);
for direct_mapping in [false, true] {
let GenesisConfigInfo {
genesis_config,
mint_keypair,
..
} = create_genesis_config(100_123_456_789);
let mut bank = Bank::new_for_tests(&genesis_config);
let mut feature_set = FeatureSet::all_enabled();
if !direct_mapping {
feature_set.deactivate(&feature_set::bpf_account_data_direct_mapping::id());
}
bank.feature_set = Arc::new(feature_set);
let bank = Arc::new(bank);
let mut bank_client = BankClient::new_shared(&bank);

let account_keypair = Keypair::new();
let invoke_program_id = load_program(
&bank_client,
&bpf_loader::id(),
&mint_keypair,
"solana_sbf_rust_invoke",
);

let mint_pubkey = mint_keypair.pubkey();
let account_metas = vec![
AccountMeta::new(mint_pubkey, true),
AccountMeta::new(account_keypair.pubkey(), false),
AccountMeta::new_readonly(invoked_program_id, false),
AccountMeta::new_readonly(invoke_program_id, false),
];
let (bank, invoked_program_id) = load_program_and_advance_slot(
&mut bank_client,
&bpf_loader::id(),
&mint_keypair,
"solana_sbf_rust_invoked",
);

for (account_size, byte_index) in [
(0, 0), // first realloc byte
(0, MAX_PERMITTED_DATA_INCREASE as u8), // last realloc byte
(2, 0), // first data byte
(2, 1), // last data byte
(2, 3), // first realloc byte
(2, 2 + MAX_PERMITTED_DATA_INCREASE as u8), // last realloc byte
] {
bank.register_recent_blockhash(&Hash::new_unique());
let account = AccountSharedData::new(42, account_size, &invoke_program_id);
bank.store_account(&account_keypair.pubkey(), &account);
let account_keypair = Keypair::new();

let instruction = Instruction::new_with_bytes(
invoke_program_id,
&[
TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT,
byte_index,
42,
42,
],
account_metas.clone(),
);
let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction);
assert_eq!(
result.unwrap_err().unwrap(),
TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified)
);
let mint_pubkey = mint_keypair.pubkey();
let account_metas = vec![
AccountMeta::new(mint_pubkey, true),
AccountMeta::new(account_keypair.pubkey(), false),
AccountMeta::new_readonly(invoked_program_id, false),
AccountMeta::new_readonly(invoke_program_id, false),
];

let instruction = Instruction::new_with_bytes(
invoke_program_id,
&[
TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_OTHER_PROGRAM,
byte_index,
42,
42,
],
account_metas.clone(),
);
let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction);
assert_eq!(
result.unwrap_err().unwrap(),
TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified)
);
for (account_size, byte_index) in [
(0, 0), // first realloc byte
(0, MAX_PERMITTED_DATA_INCREASE as u8), // last realloc byte
(2, 0), // first data byte
(2, 1), // last data byte
(2, 3), // first realloc byte
(2, 2 + MAX_PERMITTED_DATA_INCREASE as u8), // last realloc byte
] {
bank.register_recent_blockhash(&Hash::new_unique());
let account = AccountSharedData::new(42, account_size, &invoke_program_id);
bank.store_account(&account_keypair.pubkey(), &account);

let instruction = Instruction::new_with_bytes(
invoke_program_id,
&[
TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT,
byte_index,
42,
42,
],
account_metas.clone(),
);
let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction);
if (byte_index as usize) < account_size || direct_mapping {
assert_eq!(
result.unwrap_err().unwrap(),
TransactionError::InstructionError(
0,
InstructionError::ExternalAccountDataModified
)
);
} else {
// without direct mapping, changes to the realloc padding
// outside the account length are ignored
assert!(result.is_ok(), "{result:?}");
}

// assign the account to invoked_program_id, which will then assign to invoke_program_id
let mut account = bank.get_account(&account_keypair.pubkey()).unwrap();
account.set_owner(invoked_program_id);
bank.store_account(&account_keypair.pubkey(), &account);
let account = AccountSharedData::new(42, account_size, &invoke_program_id);
bank.store_account(&account_keypair.pubkey(), &account);
let instruction = Instruction::new_with_bytes(
invoke_program_id,
&[
TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_OTHER_PROGRAM,
byte_index,
42,
42,
],
account_metas.clone(),
);
let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction);
if (byte_index as usize) < account_size || direct_mapping {
assert_eq!(
result.unwrap_err().unwrap(),
TransactionError::InstructionError(
0,
InstructionError::ExternalAccountDataModified
)
);
} else {
// without direct mapping, changes to the realloc padding
// outside the account length are ignored
assert!(result.is_ok(), "{result:?}");
}

let instruction = Instruction::new_with_bytes(
invoke_program_id,
&[
TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER,
byte_index,
42,
42,
],
account_metas.clone(),
);
let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction);
assert!(result.is_ok(), "{result:?}");
let account = bank.get_account(&account_keypair.pubkey()).unwrap();
let mut data = vec![0; account.data().len()];
if (byte_index as usize) < data.len() {
data[byte_index as usize] = 42;
// assign the account to invoked_program_id, which will then assign to invoke_program_id
// let mut account = bank.get_account(&account_keypair.pubkey()).unwrap();
// account.set_owner(invoked_program_id);
// bank.store_account(&account_keypair.pubkey(), &account);
let account = AccountSharedData::new(42, account_size, &invoked_program_id);
bank.store_account(&account_keypair.pubkey(), &account);

let instruction = Instruction::new_with_bytes(
invoke_program_id,
&[
TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER,
byte_index,
42,
42,
],
account_metas.clone(),
);
let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction);
assert!(result.is_ok(), "{result:?}");
let account = bank.get_account(&account_keypair.pubkey()).unwrap();
let mut data = vec![0; account.data().len()];
if (byte_index as usize) < data.len() {
data[byte_index as usize] = 42;
}
assert_eq!(account.data(), data);
}
assert_eq!(account.data(), data);
}
}

Expand Down

0 comments on commit fa28958

Please sign in to comment.