Return error if Transaction contains writable executable or ProgramData accounts

programs/bpf/tests/programs.rs

@@ -1865,9 +1865,9 @@ fn test_program_bpf_invoke_upgradeable_via_cpi() {
         invoke_and_return,
         &[0],
         vec![
-            AccountMeta::new(program_id, false),
-            AccountMeta::new(program_id, false),
-            AccountMeta::new(clock::id(), false),
+            AccountMeta::new_readonly(program_id, false),
+            AccountMeta::new_readonly(program_id, false),
+            AccountMeta::new_readonly(clock::id(), false),
         ],
     );
 
@@ -2054,9 +2054,9 @@ fn test_program_bpf_upgrade_via_cpi() {
         invoke_and_return,
         &[0],
         vec![
-            AccountMeta::new(program_id, false),
-            AccountMeta::new(program_id, false),
-            AccountMeta::new(clock::id(), false),
+            AccountMeta::new_readonly(program_id, false),
+            AccountMeta::new_readonly(program_id, false),
+            AccountMeta::new_readonly(clock::id(), false),
         ],
     );
 

runtime/src/accounts.rs

@@ -251,9 +251,6 @@ impl Accounts {
                     }
 
                     if solana_sdk::sysvar::instructions::check_id(key) {
-                        if message.is_writable(i, demote_program_write_locks) {
-                            return Err(TransactionError::InvalidAccountIndex);
-                        }
                         Self::construct_instructions_account(
                             message,
                             feature_set
@@ -278,26 +275,49 @@ impl Accounts {
                             })
                             .unwrap_or_default();
 
-                        if account.executable() && bpf_loader_upgradeable::check_id(account.owner())
+                        if account.executable()
+                            && demote_program_write_locks
+                            && message.is_writable(i, demote_program_write_locks)
+                            && !is_upgradeable_loader_present(message)
                         {
-                            // The upgradeable loader requires the derived ProgramData account
-                            if let Ok(UpgradeableLoaderState::Program {
-                                programdata_address,
-                            }) = account.state()
-                            {
-                                if let Some(account) = self
-                                    .accounts_db
-                                    .load_with_fixed_root(ancestors, &programdata_address)
-                                    .map(|(account, _)| account)
+                            error_counters.invalid_account_index += 1;
+                            return Err(TransactionError::InvalidAccountIndex);
+                        }
+
+                        if bpf_loader_upgradeable::check_id(account.owner()) {
+                            if account.executable() {
+                                // The upgradeable loader requires the derived ProgramData account
+                                if let Ok(UpgradeableLoaderState::Program {
+                                    programdata_address,
+                                }) = account.state()
                                 {
-                                    account_deps.push((programdata_address, account));
+                                    if let Some(account) = self
+                                        .accounts_db
+                                        .load_with_fixed_root(ancestors, &programdata_address)
+                                        .map(|(account, _)| account)
+                                    {
+                                        account_deps.push((programdata_address, account));
+                                    } else {
+                                        error_counters.account_not_found += 1;
+                                        return Err(TransactionError::ProgramAccountNotFound);
+                                    }
                                 } else {
-                                    error_counters.account_not_found += 1;
-                                    return Err(TransactionError::ProgramAccountNotFound);
+                                    error_counters.invalid_program_for_execution += 1;
+                                    return Err(TransactionError::InvalidProgramForExecution);
+                                }
+                            }
+
+                            // If a ProgramData account is present, it should only be writable
+                            // if the bpf_loader_upgradeable account is also present
+                            if let Ok(UpgradeableLoaderState::ProgramData { .. }) = account.state()
+                            {
+                                if demote_program_write_locks
+                                    && message.is_writable(i, demote_program_write_locks)
+                                    && !is_upgradeable_loader_present(message)
+                                {
+                                    error_counters.invalid_account_index += 1;
+                                    return Err(TransactionError::InvalidAccountIndex);
                                 }
-                            } else {
-                                error_counters.invalid_program_for_execution += 1;
-                                return Err(TransactionError::InvalidProgramForExecution);
                             }
                         }
 
@@ -1106,6 +1126,12 @@ pub fn prepare_if_nonce_account(
     false
 }
 
+fn is_upgradeable_loader_present(message: &SanitizedMessage) -> bool {
+    message
+        .account_keys_iter()
+        .any(|&key| key == bpf_loader_upgradeable::id())
+}
+
 pub fn create_test_accounts(
     accounts: &Accounts,
     pubkeys: &mut Vec<Pubkey>,
@@ -1682,6 +1708,168 @@ mod tests {
         assert_eq!(loaded, vec![]);
     }
 
+    #[test]
+    fn test_load_accounts_executable_with_write_lock() {
+        let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new();
+        let mut error_counters = ErrorCounters::default();
+
+        let keypair = Keypair::new();
+        let key0 = keypair.pubkey();
+        let key1 = Pubkey::new(&[5u8; 32]);
+        let key2 = Pubkey::new(&[6u8; 32]);
+
+        let mut account = AccountSharedData::new(1, 0, &Pubkey::default());
+        account.set_rent_epoch(1);
+        accounts.push((key0, account));
+
+        let mut account = AccountSharedData::new(40, 1, &native_loader::id());
+        account.set_executable(true);
+        account.set_rent_epoch(1);
+        accounts.push((key1, account));
+
+        let mut account = AccountSharedData::new(40, 1, &native_loader::id());
+        account.set_executable(true);
+        account.set_rent_epoch(1);
+        accounts.push((key2, account));
+
+        let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])];
+        let mut message = Message::new_with_compiled_instructions(
+            1,
+            0,
+            1, // only one executable marked as readonly
+            vec![key0, key1, key2],
+            Hash::default(),
+            instructions,
+        );
+        let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
+        let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters);
+
+        assert_eq!(error_counters.invalid_account_index, 1);
+        assert_eq!(loaded_accounts.len(), 1);
+        assert_eq!(
+            loaded_accounts[0],
+            (Err(TransactionError::InvalidAccountIndex), None)
+        );
+
+        // Solution 1: include bpf_loader_upgradeable account
+        let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable
+        account.set_executable(true);
+        account.set_rent_epoch(1);
+        let accounts_with_upgradeable_loader = vec![
+            accounts[0].clone(),
+            accounts[1].clone(),
+            (bpf_loader_upgradeable::id(), account),
+        ];
+        message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()];
+        let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
+        let loaded_accounts =
+            load_accounts(tx, &accounts_with_upgradeable_loader, &mut error_counters);
+
+        assert_eq!(error_counters.invalid_account_index, 1);
+        assert_eq!(loaded_accounts.len(), 1);
+        let result = loaded_accounts[0].0.as_ref().unwrap();
+        assert_eq!(result.accounts[..2], accounts_with_upgradeable_loader[..2]);
+        assert_eq!(
+            result.loaders[0],
+            vec![accounts_with_upgradeable_loader[2].clone()]
+        );
+
+        // Solution 2: mark programdata as readonly
+        message.account_keys = vec![key0, key1, key2]; // revert key change
+        message.header.num_readonly_unsigned_accounts = 2; // ark both executables as readonly
+        let tx = Transaction::new(&[&keypair], message, Hash::default());
+        let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters);
+
+        assert_eq!(error_counters.invalid_account_index, 1);
+        assert_eq!(loaded_accounts.len(), 1);
+        let result = loaded_accounts[0].0.as_ref().unwrap();
+        assert_eq!(result.accounts[..2], accounts[..2]);
+        assert_eq!(result.loaders[0], vec![accounts[2].clone()]);
+    }
+
+    #[test]
+    fn test_load_accounts_programdata_with_write_lock() {
+        let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new();
+        let mut error_counters = ErrorCounters::default();
+
+        let keypair = Keypair::new();
+        let key0 = keypair.pubkey();
+        let key1 = Pubkey::new(&[5u8; 32]);
+        let key2 = Pubkey::new(&[6u8; 32]);
+
+        let mut account = AccountSharedData::new(1, 0, &Pubkey::default());
+        account.set_rent_epoch(1);
+        accounts.push((key0, account));
+
+        let program_data = UpgradeableLoaderState::ProgramData {
+            slot: 42,
+            upgrade_authority_address: None,
+        };
+        let mut account =
+            AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap();
+        account.set_rent_epoch(1);
+        accounts.push((key1, account));
+
+        let mut account = AccountSharedData::new(40, 1, &native_loader::id());
+        account.set_executable(true);
+        account.set_rent_epoch(1);
+        accounts.push((key2, account));
+
+        let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])];
+        let mut message = Message::new_with_compiled_instructions(
+            1,
+            0,
+            1, // only the program marked as readonly
+            vec![key0, key1, key2],
+            Hash::default(),
+            instructions,
+        );
+        let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
+        let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters);
+
+        assert_eq!(error_counters.invalid_account_index, 1);
+        assert_eq!(loaded_accounts.len(), 1);
+        assert_eq!(
+            loaded_accounts[0],
+            (Err(TransactionError::InvalidAccountIndex), None)
+        );
+
+        // Solution 1: include bpf_loader_upgradeable account
+        let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable
+        account.set_executable(true);
+        account.set_rent_epoch(1);
+        let accounts_with_upgradeable_loader = vec![
+            accounts[0].clone(),
+            accounts[1].clone(),
+            (bpf_loader_upgradeable::id(), account),
+        ];
+        message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()];
+        let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
+        let loaded_accounts =
+            load_accounts(tx, &accounts_with_upgradeable_loader, &mut error_counters);
+
+        assert_eq!(error_counters.invalid_account_index, 1);
+        assert_eq!(loaded_accounts.len(), 1);
+        let result = loaded_accounts[0].0.as_ref().unwrap();
+        assert_eq!(result.accounts[..2], accounts_with_upgradeable_loader[..2]);
+        assert_eq!(
+            result.loaders[0],
+            vec![accounts_with_upgradeable_loader[2].clone()]
+        );
+
+        // Solution 2: mark programdata as readonly
+        message.account_keys = vec![key0, key1, key2]; // revert key change
+        message.header.num_readonly_unsigned_accounts = 2; // extend readonly set to include programdata
+        let tx = Transaction::new(&[&keypair], message, Hash::default());
+        let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters);
+
+        assert_eq!(error_counters.invalid_account_index, 1);
+        assert_eq!(loaded_accounts.len(), 1);
+        let result = loaded_accounts[0].0.as_ref().unwrap();
+        assert_eq!(result.accounts[..2], accounts[..2]);
+        assert_eq!(result.loaders[0], vec![accounts[2].clone()]);
+    }
+
     #[test]
     fn test_accounts_account_not_found() {
         let accounts = Accounts::new_with_config_for_tests(