diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index a53629e0a7fba2..5d55c9690e8d94 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1,6 +1,7 @@ use { super::*, crate::declare_syscall, + solana_rbpf::memory_region::MemoryState, solana_sdk::{ feature_set::enable_bpf_loader_set_authority_checked_ix, stable_layout::stable_instruction::StableInstruction, @@ -1147,7 +1148,31 @@ fn update_caller_account( if region.host_addr.get() != callee_ptr { region.host_addr.set(callee_ptr); } + + match ( + region.state.get(), + callee_account.can_data_be_changed().is_ok(), + ) { + (MemoryState::Readable, true) => { + if callee_account.is_shared() { + // If the account is still shared it means it wasn't written to yet during this + // transaction. We map it as CoW and it'll be copied the first time something + // tries to write into it. + let index_in_transaction = callee_account.get_index_in_transaction(); + region + .state + .set(MemoryState::Cow(index_in_transaction as u64)); + } else { + region.state.set(MemoryState::Writable); + } + } + (MemoryState::Writable | MemoryState::Cow(_), false) => { + region.state.set(MemoryState::Readable) + } + _ => {} + } } + let prev_len = *caller_account.ref_to_len_in_vm as usize; let post_len = callee_account.get_data().len(); if prev_len != post_len { diff --git a/programs/sbf/rust/invoke/src/instructions.rs b/programs/sbf/rust/invoke/src/instructions.rs index db8be12dea5619..39e8c1f0f4c089 100644 --- a/programs/sbf/rust/invoke/src/instructions.rs +++ b/programs/sbf/rust/invoke/src/instructions.rs @@ -21,6 +21,10 @@ pub const TEST_RETURN_DATA_TOO_LARGE: u8 = 18; pub const TEST_DUPLICATE_PRIVILEGE_ESCALATION_SIGNER: u8 = 19; pub const TEST_DUPLICATE_PRIVILEGE_ESCALATION_WRITABLE: u8 = 20; pub const TEST_MAX_ACCOUNT_INFOS_EXCEEDED: u8 = 21; +pub const TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT: u8 = 22; +pub const TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT_NESTED: u8 = 23; +pub const TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_OTHER_PROGRAM: u8 = 24; +pub const TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER: u8 = 25; pub const MINT_INDEX: usize = 0; pub const ARGUMENT_INDEX: usize = 1; diff --git a/programs/sbf/rust/invoke/src/processor.rs b/programs/sbf/rust/invoke/src/processor.rs index dce6492ecefff0..23c579d16b22bc 100644 --- a/programs/sbf/rust/invoke/src/processor.rs +++ b/programs/sbf/rust/invoke/src/processor.rs @@ -16,7 +16,7 @@ use { syscalls::{ MAX_CPI_ACCOUNT_INFOS, MAX_CPI_INSTRUCTION_ACCOUNTS, MAX_CPI_INSTRUCTION_DATA_LEN, }, - system_instruction, + system_instruction, system_program, }, solana_sbf_rust_invoked::instructions::*, }; @@ -688,7 +688,86 @@ fn process_instruction( invoked_instruction.accounts[1].is_writable = true; invoke(&invoked_instruction, accounts)?; } - _ => panic!(), + TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT => { + msg!("Test forbid write after ownership change to system account"); + invoke( + &create_instruction( + *program_id, + &[ + (program_id, false, false), + (accounts[ARGUMENT_INDEX].key, true, false), + ], + vec![ + TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT_NESTED, + 42, + 42, + 42, + ], + ), + accounts, + ) + .unwrap(); + let account = &accounts[ARGUMENT_INDEX]; + // this should cause the tx to fail with ReadonlyDataModified since + // the system program now owns the account + account.data.borrow_mut()[0] = 42; + } + TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_SYSTEM_ACCOUNT_NESTED => { + msg!("Assigning to system account"); + let account = &accounts[ARGUMENT_INDEX]; + account.data.borrow_mut().fill(0); + account.assign(&system_program::id()); + } + TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_OTHER_PROGRAM => { + msg!("Test forbid write after ownership change to other program"); + let account = &accounts[ARGUMENT_INDEX]; + let invoked_program_id = accounts[INVOKED_PROGRAM_INDEX].key; + account.realloc(2, false).unwrap(); + account.data.borrow_mut().fill(0); + account.assign(invoked_program_id); + invoke( + &create_instruction( + *invoked_program_id, + &[ + (accounts[ARGUMENT_INDEX].key, true, false), + (invoked_program_id, false, false), + ], + vec![WRITE_ACCOUNT, 1], + ), + accounts, + ) + .unwrap(); + let mut data = account.data.borrow_mut(); + assert_eq!(data[0], 1); + assert_eq!(data[1], 0); + // this should cause the tx to fail with ReadonlyDataModified since + // invoked_program_id now owns the account + data[1] = 42; + } + TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER => { + msg!("Test allow write after ownership change to caller"); + const INVOKE_PROGRAM_INDEX: usize = 3; + let account = &accounts[ARGUMENT_INDEX]; + let invoked_program_id = accounts[INVOKED_PROGRAM_INDEX].key; + let invoke_program_id = accounts[INVOKE_PROGRAM_INDEX].key; + invoke( + &create_instruction( + *invoked_program_id, + &[ + (accounts[ARGUMENT_INDEX].key, true, false), + (invoked_program_id, false, false), + (invoke_program_id, false, false), + ], + vec![ASSIGN_ACCOUNT_TO_CALLER], + ), + accounts, + ) + .unwrap(); + let mut data = account.data.borrow_mut(); + // this should succeed since the callee gave us ownership of the account + data[0] = 42; + } + _ => panic!("unexpected program data"), } Ok(()) diff --git a/programs/sbf/rust/invoked/src/instructions.rs b/programs/sbf/rust/invoked/src/instructions.rs index 9f98da7d92aa52..46a4f1d9f2f45c 100644 --- a/programs/sbf/rust/invoked/src/instructions.rs +++ b/programs/sbf/rust/invoked/src/instructions.rs @@ -19,6 +19,7 @@ pub const VERIFY_PRIVILEGE_DEESCALATION_ESCALATION_WRITABLE: u8 = 10; pub const WRITE_ACCOUNT: u8 = 11; pub const CREATE_AND_INIT: u8 = 12; pub const SET_RETURN_DATA: u8 = 13; +pub const ASSIGN_ACCOUNT_TO_CALLER: u8 = 14; pub fn create_instruction( program_id: Pubkey, diff --git a/programs/sbf/rust/invoked/src/processor.rs b/programs/sbf/rust/invoked/src/processor.rs index 42a427c74db4f0..610e3eafef9017 100644 --- a/programs/sbf/rust/invoked/src/processor.rs +++ b/programs/sbf/rust/invoked/src/processor.rs @@ -297,6 +297,14 @@ fn process_instruction( set_return_data(b"Set by invoked"); } + ASSIGN_ACCOUNT_TO_CALLER => { + msg!("Assigning account to caller"); + const ARGUMENT_INDEX: usize = 0; + const CALLER_PROGRAM_ID: usize = 2; + let account = &accounts[ARGUMENT_INDEX]; + let caller_program_id = accounts[CALLER_PROGRAM_ID].key; + account.assign(caller_program_id); + } _ => panic!(), } diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 9a3910f4f7db48..ecbd079d02a9d4 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -3933,3 +3933,98 @@ fn test_program_sbf_inner_instruction_alignment_checks() { let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction.clone()); assert!(result.is_ok()); } + +#[test] +#[cfg(feature = "sbf_rust")] +fn test_cpi_account_ownership_writability() { + use solana_sbf_rust_invoke::instructions::TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_OTHER_PROGRAM; + + 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", + ); + let argument_keypair = Keypair::new(); + let account = AccountSharedData::new(42, 2, &invoke_program_id); + bank.store_account(&argument_keypair.pubkey(), &account); + + let mint_pubkey = mint_keypair.pubkey(); + let account_metas = vec![ + AccountMeta::new(mint_pubkey, true), + AccountMeta::new(argument_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_SYSTEM_ACCOUNT, + 42, + 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 instruction = Instruction::new_with_bytes( + invoke_program_id, + &[ + TEST_FORBID_WRITE_AFTER_OWNERSHIP_CHANGE_TO_OTHER_PROGRAM, + 42, + 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) + ); + + // assign the account to invoked_program_id, which will then assign to invoke_program_id + let mut account = bank.get_account(&argument_keypair.pubkey()).unwrap(); + account.set_owner(invoked_program_id); + + assert_eq!(account.data(), &[0, 0]); + let instruction = Instruction::new_with_bytes( + invoke_program_id, + &[ + TEST_ALLOW_WRITE_AFTER_OWNERSHIP_CHANGE_TO_CALLER, + 42, + 42, + 42, + ], + account_metas, + ); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + assert!(result.is_ok(), "{result:?}"); + let account = bank.get_account(&argument_keypair.pubkey()).unwrap(); + assert_eq!(account.data(), &[42, 0]); +}