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

@@ -243,6 +243,7 @@ impl Accounts {
             let rent_for_sysvars = feature_set.is_active(&feature_set::rent_for_sysvars::id());
             let demote_program_write_locks =
                 feature_set.is_active(&feature_set::demote_program_write_locks::id());
+            let is_upgradeable_loader_present = is_upgradeable_loader_present(message);
 
             for (i, key) in message.account_keys_iter().enumerate() {
                 let account = if message.is_non_loader_key(i) {
@@ -251,9 +252,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 +276,48 @@ 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
                         {
-                            // 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_writable_account += 1;
+                            return Err(TransactionError::InvalidWritableAccount);
+                        }
+
+                        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);
+                                }
+                            } else if demote_program_write_locks
+                                && message.is_writable(i, demote_program_write_locks)
+                                && !is_upgradeable_loader_present
+                            {
+                                // 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()
+                                {
+                                    error_counters.invalid_writable_account += 1;
+                                    return Err(TransactionError::InvalidWritableAccount);
                                 }
-                            } 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_writable_account, 1);
+        assert_eq!(loaded_accounts.len(), 1);
+        assert_eq!(
+            loaded_accounts[0],
+            (Err(TransactionError::InvalidWritableAccount), 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_writable_account, 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_writable_account, 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_writable_account, 1);
+        assert_eq!(loaded_accounts.len(), 1);
+        assert_eq!(
+            loaded_accounts[0],
+            (Err(TransactionError::InvalidWritableAccount), 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_writable_account, 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_writable_account, 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(

runtime/src/accounts_db.rs

@@ -175,6 +175,7 @@ pub struct ErrorCounters {
     pub invalid_account_index: usize,
     pub invalid_program_for_execution: usize,
     pub not_allowed_during_cluster_maintenance: usize,
+    pub invalid_writable_account: usize,
 }
 
 #[derive(Default, Debug)]

runtime/src/bank.rs

@@ -189,7 +189,7 @@ impl ExecuteTimings {
 }
 
 type BankStatusCache = StatusCache<Result<()>>;
-#[frozen_abi(digest = "GT81Hdwrh73i55ScQvFqmzeHfUL42yxuavZods8VyzGc")]
+#[frozen_abi(digest = "5Br3PNyyX1L7XoS4jYLt5JTeMXowLSsu7v9LhokC8vnq")]
 pub type BankSlotDelta = SlotDelta<Result<()>>;
 type TransactionAccountRefCells = Vec<(Pubkey, Rc<RefCell<AccountSharedData>>)>;
 type TransactionLoaderRefCells = Vec<Vec<(Pubkey, Rc<RefCell<AccountSharedData>>)>>;
@@ -3119,6 +3119,12 @@ impl Bank {
                 error_counters.not_allowed_during_cluster_maintenance
             );
         }
+        if 0 != error_counters.invalid_writable_account {
+            inc_new_counter_info!(
+                "bank-process_transactions-error-invalid_writable_account",
+                error_counters.invalid_writable_account
+            );
+        }
     }
 
     /// Converts Accounts into RefCell<AccountSharedData>, this involves moving

sdk/src/transaction/mod.rs

@@ -116,6 +116,10 @@ pub enum TransactionError {
     /// Transaction version is unsupported
     #[error("Transaction version is unsupported")]
     UnsupportedVersion,
+
+    /// Transaction loads a writable account that cannot be written
+    #[error("Transaction loads a writable account that cannot be written")]
+    InvalidWritableAccount,
 }
 
 pub type Result<T> = result::Result<T, TransactionError>;

storage-proto/proto/transaction_by_addr.proto

@@ -43,6 +43,7 @@ enum TransactionErrorType {
     ACCOUNT_BORROW_OUTSTANDING_TX = 16;
     WOULD_EXCEED_MAX_BLOCK_COST_LIMIT = 17;
     UNSUPPORTED_VERSION = 18;
+    INVALID_WRITABLE_ACCOUNT = 19;
 }
 
 message InstructionError {

storage-proto/src/convert.rs

@@ -550,6 +550,7 @@ impl TryFrom<tx_by_addr::TransactionError> for TransactionError {
             16 => TransactionError::AccountBorrowOutstanding,
             17 => TransactionError::WouldExceedMaxBlockCostLimit,
             18 => TransactionError::UnsupportedVersion,
+            19 => TransactionError::InvalidWritableAccount,
             _ => return Err("Invalid TransactionError"),
         })
     }
@@ -614,6 +615,9 @@ impl From<TransactionError> for tx_by_addr::TransactionError {
                 TransactionError::UnsupportedVersion => {
                     tx_by_addr::TransactionErrorType::UnsupportedVersion
                 }
+                TransactionError::InvalidWritableAccount => {
+                    tx_by_addr::TransactionErrorType::InvalidWritableAccount
+                }
             } as i32,
             instruction_error: match transaction_error {
                 TransactionError::InstructionError(index, ref instruction_error) => {