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

Feature - Remove accounts executable flag checks #2182

Merged
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
11 changes: 9 additions & 2 deletions program-runtime/src/invoke_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ use {
sysvar_cache::SysvarCache,
},
solana_compute_budget::compute_budget::ComputeBudget,
solana_feature_set::{move_precompile_verification_to_svm, FeatureSet},
solana_feature_set::{
move_precompile_verification_to_svm, remove_accounts_executable_flag_checks, FeatureSet,
},
solana_log_collector::{ic_msg, LogCollector},
solana_measure::measure::Measure,
solana_rbpf::{
Expand Down Expand Up @@ -435,7 +437,12 @@ impl<'a> InvokeContext<'a> {
})?;
let borrowed_program_account = instruction_context
.try_borrow_instruction_account(self.transaction_context, program_account_index)?;
if !borrowed_program_account.is_executable() {
#[allow(deprecated)]
if !self
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
&& !borrowed_program_account.is_executable()
{
ic_msg!(self, "Account {} is not executable", callee_program_id);
return Err(InstructionError::AccountNotExecutable);
}
Expand Down
138 changes: 109 additions & 29 deletions programs/bpf_loader/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use {
solana_compute_budget::compute_budget::MAX_INSTRUCTION_STACK_DEPTH,
solana_feature_set::{
bpf_account_data_direct_mapping, enable_bpf_loader_set_authority_checked_ix,
remove_accounts_executable_flag_checks,
},
solana_log_collector::{ic_logger_msg, ic_msg, LogCollector},
solana_measure::measure::Measure,
Expand Down Expand Up @@ -423,14 +424,28 @@ pub fn process_instruction_inner(
Err(InstructionError::UnsupportedProgramId)
} else {
ic_logger_msg!(log_collector, "Invalid BPF loader id");
Err(InstructionError::IncorrectProgramId)
Err(
if invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
{
InstructionError::UnsupportedProgramId
} else {
InstructionError::IncorrectProgramId
},
)
}
.map(|_| 0)
.map_err(|error| Box::new(error) as Box<dyn std::error::Error>);
}

// Program Invocation
if !program_account.is_executable() {
#[allow(deprecated)]
if !invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
&& !program_account.is_executable()
{
ic_logger_msg!(log_collector, "Program is not executable");
return Err(Box::new(InstructionError::IncorrectProgramId));
}
Expand All @@ -441,7 +456,14 @@ pub fn process_instruction_inner(
.find(program_account.get_key())
.ok_or_else(|| {
ic_logger_msg!(log_collector, "Program is not cached");
InstructionError::InvalidAccountData
if invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
{
InstructionError::UnsupportedProgramId
} else {
InstructionError::InvalidAccountData
}
})?;
drop(program_account);
get_or_create_executor_time.stop();
Expand All @@ -456,10 +478,28 @@ pub fn process_instruction_inner(
| ProgramCacheEntryType::Closed
| ProgramCacheEntryType::DelayVisibility => {
ic_logger_msg!(log_collector, "Program is not deployed");
Err(Box::new(InstructionError::InvalidAccountData) as Box<dyn std::error::Error>)
let instruction_error = if invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
{
InstructionError::UnsupportedProgramId
} else {
InstructionError::InvalidAccountData
};
Err(Box::new(instruction_error) as Box<dyn std::error::Error>)
}
ProgramCacheEntryType::Loaded(executable) => execute(executable, invoke_context),
_ => Err(Box::new(InstructionError::IncorrectProgramId) as Box<dyn std::error::Error>),
_ => {
let instruction_error = if invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
{
InstructionError::UnsupportedProgramId
} else {
InstructionError::IncorrectProgramId
};
Err(Box::new(instruction_error) as Box<dyn std::error::Error>)
}
}
.map(|_| 0)
}
Expand Down Expand Up @@ -718,7 +758,12 @@ fn process_loader_upgradeable_instruction(

let program =
instruction_context.try_borrow_instruction_account(transaction_context, 1)?;
if !program.is_executable() {
#[allow(deprecated)]
if !invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
&& !program.is_executable()
{
ic_logger_msg!(log_collector, "Program account not executable");
return Err(InstructionError::AccountNotExecutable);
}
Expand Down Expand Up @@ -1457,13 +1502,20 @@ pub fn execute<'a, 'b: 'a>(
instruction_account_index as IndexOfAccount,
)?;

error = EbpfError::SyscallError(Box::new(if account.is_executable() {
InstructionError::ExecutableDataModified
} else if account.is_writable() {
InstructionError::ExternalAccountDataModified
} else {
InstructionError::ReadonlyDataModified
}));
error = EbpfError::SyscallError(Box::new(
#[allow(deprecated)]
if !invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
&& account.is_executable()
{
InstructionError::ExecutableDataModified
} else if account.is_writable() {
InstructionError::ExternalAccountDataModified
} else {
InstructionError::ReadonlyDataModified
},
));
}
}
}
Expand Down Expand Up @@ -1636,7 +1688,7 @@ mod tests {
fn test_bpf_loader_invoke_main() {
let loader_id = bpf_loader::id();
let program_id = Pubkey::new_unique();
let mut program_account =
let program_account =
load_program_account_from_elf(&loader_id, "test_elfs/out/noop_aligned.so");
let parameter_id = Pubkey::new_unique();
let parameter_account = AccountSharedData::new(1, 0, &loader_id);
Expand Down Expand Up @@ -1686,7 +1738,7 @@ mod tests {
&[],
vec![
(program_id, program_account.clone()),
(parameter_id, parameter_account),
(parameter_id, parameter_account.clone()),
],
vec![parameter_meta.clone(), parameter_meta],
Ok(()),
Expand All @@ -1697,7 +1749,7 @@ mod tests {
&loader_id,
vec![0],
&[],
vec![(program_id, program_account.clone())],
vec![(program_id, program_account)],
Vec::new(),
Err(InstructionError::ProgramFailedToComplete),
Entrypoint::vm,
Expand All @@ -1709,14 +1761,29 @@ mod tests {
);

// Case: Account not a program
program_account.set_executable(false);
mock_process_instruction(
&loader_id,
vec![0],
&[],
vec![(program_id, parameter_account.clone())],
Vec::new(),
Err(InstructionError::IncorrectProgramId),
Entrypoint::vm,
|invoke_context| {
let mut feature_set = invoke_context.get_feature_set().clone();
feature_set.deactivate(&remove_accounts_executable_flag_checks::id());
invoke_context.mock_set_feature_set(Arc::new(feature_set));
test_utils::load_all_invoked_programs(invoke_context);
},
|_invoke_context| {},
);
process_instruction(
&loader_id,
&[0],
&[],
vec![(program_id, program_account)],
vec![(program_id, parameter_account)],
Vec::new(),
Err(InstructionError::IncorrectProgramId),
Err(InstructionError::UnsupportedProgramId),
);
}

Expand Down Expand Up @@ -2379,22 +2446,35 @@ mod tests {
);

// Case: Program account not executable
let (mut transaction_accounts, instruction_accounts) = get_accounts(
let (transaction_accounts, mut instruction_accounts) = get_accounts(
&buffer_address,
&upgrade_authority_address,
&upgrade_authority_address,
&elf_orig,
&elf_new,
);
transaction_accounts
.get_mut(1)
.unwrap()
.1
.set_executable(false);
process_instruction(
transaction_accounts,
instruction_accounts,
*instruction_accounts.get_mut(1).unwrap() = instruction_accounts.get(2).unwrap().clone();
let instruction_data = bincode::serialize(&UpgradeableLoaderInstruction::Upgrade).unwrap();
mock_process_instruction(
&bpf_loader_upgradeable::id(),
Vec::new(),
&instruction_data,
transaction_accounts.clone(),
instruction_accounts.clone(),
Err(InstructionError::AccountNotExecutable),
Entrypoint::vm,
|invoke_context| {
let mut feature_set = invoke_context.get_feature_set().clone();
feature_set.deactivate(&remove_accounts_executable_flag_checks::id());
invoke_context.mock_set_feature_set(Arc::new(feature_set));
test_utils::load_all_invoked_programs(invoke_context);
},
|_invoke_context| {},
);
process_instruction(
transaction_accounts.clone(),
instruction_accounts.clone(),
Err(InstructionError::InvalidAccountData),
);

// Case: Program account now owned by loader
Expand Down Expand Up @@ -3593,7 +3673,7 @@ mod tests {
(program_address, program_account.clone()),
],
Vec::new(),
Err(InstructionError::InvalidAccountData),
Err(InstructionError::UnsupportedProgramId),
);

// Case: Reopen should fail
Expand Down
2 changes: 2 additions & 0 deletions programs/bpf_loader/src/serialization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ fn serialize_parameters_unaligned(
s.write::<u64>((account.get_data().len() as u64).to_le());
let vm_data_addr = s.write_account(&mut account)?;
let vm_owner_addr = s.write_all(account.get_owner().as_ref());
#[allow(deprecated)]
s.write::<u8>(account.is_executable() as u8);
s.write::<u64>((account.get_rent_epoch()).to_le());
accounts_metadata.push(SerializedAccountMetadata {
Expand Down Expand Up @@ -467,6 +468,7 @@ fn serialize_parameters_aligned(
s.write::<u8>(NON_DUP_MARKER);
s.write::<u8>(borrowed_account.is_signer() as u8);
s.write::<u8>(borrowed_account.is_writable() as u8);
#[allow(deprecated)]
s.write::<u8>(borrowed_account.is_executable() as u8);
s.write_all(&[0u8, 0, 0, 0]);
let vm_key_addr = s.write_all(borrowed_account.get_key().as_ref());
Expand Down
1 change: 1 addition & 0 deletions programs/bpf_loader/src/syscalls/cpi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -890,6 +890,7 @@ where
.transaction_context
.get_key_of_account_at_index(instruction_account.index_in_transaction)?;

#[allow(deprecated)]
if callee_account.is_executable() {
// Use the known account
consume_compute_meter(
Expand Down
1 change: 1 addition & 0 deletions programs/loader-v4/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ pub fn process_instruction_truncate(
LoaderV4State::program_data_offset().saturating_add(new_size as usize),
)?;
if is_initialization {
program.set_executable(true)?;
let state = get_state_mut(program.get_data_mut()?)?;
state.slot = 0;
state.status = LoaderV4Status::Retracted;
Expand Down
16 changes: 8 additions & 8 deletions programs/sbf/tests/programs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -918,15 +918,15 @@ fn test_program_sbf_invoke_sanity() {

do_invoke_failure_test_local(
TEST_PPROGRAM_NOT_OWNED_BY_LOADER,
TransactionError::InstructionError(0, InstructionError::AccountNotExecutable),
&[],
TransactionError::InstructionError(0, InstructionError::UnsupportedProgramId),
&[argument_keypair.pubkey()],
None,
);

do_invoke_failure_test_local(
TEST_PPROGRAM_NOT_EXECUTABLE,
TransactionError::InstructionError(0, InstructionError::AccountNotExecutable),
&[],
TransactionError::InstructionError(0, InstructionError::UnsupportedProgramId),
&[unexecutable_program_keypair.pubkey()],
None,
);

Expand Down Expand Up @@ -1895,7 +1895,7 @@ fn test_program_sbf_invoke_in_same_tx_as_deployment() {
let (result, _, _) = process_transaction_and_record_inner(&bank, tx);
assert_eq!(
result.unwrap_err(),
TransactionError::InstructionError(2, InstructionError::InvalidAccountData),
TransactionError::InstructionError(2, InstructionError::UnsupportedProgramId),
);
}
}
Expand Down Expand Up @@ -2006,7 +2006,7 @@ fn test_program_sbf_invoke_in_same_tx_as_redeployment() {
let (result, _, _) = process_transaction_and_record_inner(&bank, tx);
assert_eq!(
result.unwrap_err(),
TransactionError::InstructionError(1, InstructionError::InvalidAccountData),
TransactionError::InstructionError(1, InstructionError::UnsupportedProgramId),
);
}
}
Expand Down Expand Up @@ -2101,7 +2101,7 @@ fn test_program_sbf_invoke_in_same_tx_as_undeployment() {
let (result, _, _) = process_transaction_and_record_inner(&bank, tx);
assert_eq!(
result.unwrap_err(),
TransactionError::InstructionError(1, InstructionError::InvalidAccountData),
TransactionError::InstructionError(1, InstructionError::UnsupportedProgramId),
);
}
}
Expand Down Expand Up @@ -4650,7 +4650,7 @@ fn test_deny_executable_write() {
let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction);
assert_eq!(
result.unwrap_err().unwrap(),
TransactionError::InstructionError(0, InstructionError::ExecutableDataModified)
TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified)
);
}
}
Expand Down
4 changes: 2 additions & 2 deletions runtime/src/bank/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7231,7 +7231,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
bank.process_transaction(&transaction),
Err(TransactionError::InstructionError(
0,
InstructionError::InvalidAccountData
InstructionError::UnsupportedProgramId
)),
);
{
Expand All @@ -7255,7 +7255,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
bank.process_transaction(&transaction),
Err(TransactionError::InstructionError(
0,
InstructionError::InvalidAccountData,
InstructionError::UnsupportedProgramId,
)),
);
{
Expand Down
5 changes: 5 additions & 0 deletions sdk/feature-set/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -869,6 +869,10 @@ pub mod reenable_sbpf_v1_execution {
solana_pubkey::declare_id!("TestFeature21111111111111111111111111111111");
}

pub mod remove_accounts_executable_flag_checks {
solana_pubkey::declare_id!("FfgtauHUWKeXTzjXkua9Px4tNGBFHKZ9WaigM5VbbzFx");
}

lazy_static! {
/// Map of feature identifiers to user-visible description
pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
Expand Down Expand Up @@ -1081,6 +1085,7 @@ lazy_static! {
(partitioned_epoch_rewards_superfeature::id(), "replaces enable_partitioned_epoch_reward to enable partitioned rewards at epoch boundary SIMD-0118"),
(disable_sbpf_v1_execution::id(), "Disables execution of SBPFv1 programs"),
(reenable_sbpf_v1_execution::id(), "Re-enables execution of SBPFv1 programs"),
(remove_accounts_executable_flag_checks::id(), "Remove checks of accounts is_executable flag SIMD-0162"),
/*************** ADD NEW FEATURES HERE ***************/
]
.iter()
Expand Down
Loading
Loading