Skip to content

Commit

Permalink
Feature - Remove accounts executable flag checks (#2182)
Browse files Browse the repository at this point in the history 
* Adds the feature.

* Adds the feature gate logic.

* Adjusts tests.

* Adds deprecation attributes.

* Set is_executable flag upon initialization in loader-v4.
  • Loading branch information
@Lichtso
Lichtso authored Nov 4, 2024
1 parent 2c3d436 commit 3bf9c2e
Show file tree
Hide file tree
Showing 11 changed files with 183 additions and 49 deletions.
11 changes: 9 additions & 2 deletions program-runtime/src/invoke_context.rs
View file
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
View file
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
View file
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
View file
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
View file
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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -921,15 +921,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 @@ -1898,7 +1898,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 @@ -2009,7 +2009,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 @@ -2104,7 +2104,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 @@ -4653,7 +4653,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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7230,7 +7230,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
bank.process_transaction(&transaction),
Err(TransactionError::InstructionError(
0,
InstructionError::InvalidAccountData
InstructionError::UnsupportedProgramId
)),
);
{
Expand All @@ -7254,7 +7254,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
View file
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

0 comments on commit 3bf9c2e

Please sign in to comment.