bpf_loader: direct_mapping: map AccessViolation -> InstructionError

Return the proper ReadonlyDataModified / ExecutableDataModified /
ExternalAccountDataModified depending on where the violation occurs

programs/bpf_loader/src/lib.rs

@@ -1494,12 +1494,19 @@ fn execute<'a, 'b: 'a>(
     let log_collector = invoke_context.get_log_collector();
     let transaction_context = &invoke_context.transaction_context;
     let instruction_context = transaction_context.get_current_instruction_context()?;
-    let program_id = *instruction_context.get_last_program_key(transaction_context)?;
+    let (program_id, is_loader_deprecated) = {
+        let program_account =
+            instruction_context.try_borrow_last_program_account(transaction_context)?;
+        (
+            *program_account.get_key(),
+            *program_account.get_owner() == bpf_loader_deprecated::id(),
+        )
+    };
     #[cfg(any(target_os = "windows", not(target_arch = "x86_64")))]
     let use_jit = false;
     #[cfg(all(not(target_os = "windows"), target_arch = "x86_64"))]
     let use_jit = executable.get_compiled_program().is_some();
-    let bpf_account_data_direct_mapping = invoke_context
+    let direct_mapping = invoke_context
         .feature_set
         .is_active(&bpf_account_data_direct_mapping::id());
 
@@ -1510,18 +1517,22 @@ fn execute<'a, 'b: 'a>(
         invoke_context
             .feature_set
             .is_active(&cap_bpf_program_instruction_accounts::ID),
-        !bpf_account_data_direct_mapping,
+        !direct_mapping,
     )?;
     serialize_time.stop();
 
-    // save the account addresses so in case of AccessViolation below we can
-    // map to InstructionError::ReadonlyDataModified, which is easier to
-    // diagnose from developers
-    let account_region_addrs = regions
+    // save the account addresses so in case we hit an AccessViolation error we
+    // can map to a more specific error
+    let account_region_addrs = accounts_metadata
         .iter()
-        .map(|r| r.vm_addr..r.vm_addr.saturating_add(r.len))
+        .map(|m| {
+            let mut vm_end = m.vm_data_addr.saturating_add(m.original_data_len as u64);
+            if !is_loader_deprecated {
+                vm_end = vm_end.saturating_add(MAX_PERMITTED_DATA_INCREASE as u64)
+            }
+            m.vm_data_addr..vm_end
+        })
         .collect::<Vec<_>>();
-    let addr_is_account_data = |addr: u64| account_region_addrs.iter().any(|r| r.contains(&addr));
 
     let mut create_vm_time = Measure::start("create_vm");
     let mut execute_time;
@@ -1573,22 +1584,43 @@ fn execute<'a, 'b: 'a>(
                 };
                 Err(Box::new(error) as Box<dyn std::error::Error>)
             }
-            ProgramResult::Err(error) => {
-                let error = match error.downcast_ref() {
-                    Some(EbpfError::AccessViolation(
+            ProgramResult::Err(mut error) => {
+                if direct_mapping {
+                    if let Some(EbpfError::AccessViolation(
                         _pc,
                         AccessType::Store,
                         address,
                         _size,
                         _section_name,
-                    )) if addr_is_account_data(*address) => {
-                        // We can get here if direct_mapping is enabled and a program tries to
-                        // write to a readonly account. Map the error to ReadonlyDataModified so
-                        // it's easier for devs to diagnose what happened.
-                        Box::new(InstructionError::ReadonlyDataModified)
+                    )) = error.downcast_ref()
+                    {
+                        // If direct_mapping is enabled and a program tries to write to a readonly
+                        // region we'll get a memory access violation. Map it to a more specific
+                        // error so it's easier for developers to see what happened.
+                        if let Some((instruction_account_index, _)) = account_region_addrs
+                            .iter()
+                            .enumerate()
+                            .find(|(_, vm_region)| vm_region.contains(address))
+                        {
+                            let transaction_context = &invoke_context.transaction_context;
+                            let instruction_context =
+                                transaction_context.get_current_instruction_context()?;
+
+                            let account = instruction_context.try_borrow_instruction_account(
+                                transaction_context,
+                                instruction_account_index as IndexOfAccount,
+                            )?;
+
+                            error = Box::new(if account.is_executable() {
+                                InstructionError::ExecutableDataModified
+                            } else if account.is_writable() {
+                                InstructionError::ExternalAccountDataModified
+                            } else {
+                                InstructionError::ReadonlyDataModified
+                            })
+                        }
                     }
-                    _ => error,
-                };
+                }
                 Err(error)
             }
             _ => Ok(()),
@@ -1614,12 +1646,8 @@ fn execute<'a, 'b: 'a>(
 
     let mut deserialize_time = Measure::start("deserialize");
     let execute_or_deserialize_result = execution_result.and_then(|_| {
-        deserialize_parameters(
-            invoke_context,
-            parameter_bytes.as_slice(),
-            !bpf_account_data_direct_mapping,
-        )
-        .map_err(|error| Box::new(error) as Box<dyn std::error::Error>)
+        deserialize_parameters(invoke_context, parameter_bytes.as_slice(), !direct_mapping)
+            .map_err(|error| Box::new(error) as Box<dyn std::error::Error>)
     });
     deserialize_time.stop();