Fix incorrect nonoverlapping test in sol_memcpy (backport #21007) (#23512)

* Fix incorrect nonoverlapping test in sol_memcpy (#21007)

Thanks!

(cherry picked from commit df2b448993)

# Conflicts:
#	programs/bpf_loader/src/syscalls.rs
#	sdk/program/src/program_stubs.rs
#	sdk/src/feature_set.rs

* resolve conflicts

Co-authored-by: Brian Anderson <andersrb@gmail.com>
Co-authored-by: Jack May <jack@solana.com>

programs/bpf_loader/src/syscalls.rs

@@ -22,10 +22,10 @@ use {
         entrypoint::{BPF_ALIGN_OF_U128, MAX_PERMITTED_DATA_INCREASE, SUCCESS},
         feature_set::{
             add_get_processed_sibling_instruction_syscall, blake3_syscall_enabled,
-            disable_fees_sysvar, do_support_realloc, libsecp256k1_0_5_upgrade_enabled,
+            disable_fees_sysvar, do_support_realloc, fixed_memcpy_nonoverlapping_check,
-            prevent_calling_precompiles_as_programs, return_data_syscall_enabled,
+            libsecp256k1_0_5_upgrade_enabled, prevent_calling_precompiles_as_programs,
-            secp256k1_recover_syscall_enabled, sol_log_data_syscall_enabled,
+            return_data_syscall_enabled, secp256k1_recover_syscall_enabled,
-            update_syscall_base_costs,
+            sol_log_data_syscall_enabled, update_syscall_base_costs,
         },
         hash::{Hasher, HASH_BYTES},
         instruction::{
@@ -37,6 +37,7 @@ use {
         native_loader,
         precompiles::is_precompile,
         program::MAX_RETURN_DATA,
+        program_stubs::is_nonoverlapping,
         pubkey::{Pubkey, PubkeyError, MAX_SEEDS, MAX_SEED_LEN},
         secp256k1_recover::{
             Secp256k1RecoverError, SECP256K1_PUBLIC_KEY_LENGTH, SECP256K1_SIGNATURE_LENGTH,
@@ -1368,7 +1369,9 @@ impl<'a, 'b> SyscallObject<BpfError> for SyscallKeccak256<'a, 'b> {
     }
 }
 
-fn check_overlapping(src_addr: u64, dst_addr: u64, n: u64) -> bool {
+/// This function is incorrect due to arithmetic overflow and only exists for
+/// backwards compatibility. Instead use program_stubs::is_nonoverlapping.
+fn check_overlapping_do_not_use(src_addr: u64, dst_addr: u64, n: u64) -> bool {
     (src_addr <= dst_addr && src_addr + n > dst_addr)
         || (dst_addr <= src_addr && dst_addr + n > src_addr)
 }
@@ -1406,10 +1409,28 @@ impl<'a, 'b> SyscallObject<BpfError> for SyscallMemcpy<'a, 'b> {
         memory_mapping: &MemoryMapping,
         result: &mut Result<u64, EbpfError<BpfError>>,
     ) {
-        if check_overlapping(src_addr, dst_addr, n) {
+        let invoke_context = question_mark!(
+            self.invoke_context
+                .try_borrow()
+                .map_err(|_| SyscallError::InvokeContextBorrowFailed),
+            result
+        );
+        let use_fixed_nonoverlapping_check = invoke_context
+            .feature_set
+            .is_active(&fixed_memcpy_nonoverlapping_check::id());
+
+        #[allow(clippy::collapsible_else_if)]
+        if use_fixed_nonoverlapping_check {
+            if !is_nonoverlapping(src_addr, dst_addr, n) {
                 *result = Err(SyscallError::CopyOverlapping.into());
                 return;
             }
+        } else {
+            if check_overlapping_do_not_use(src_addr, dst_addr, n) {
+                *result = Err(SyscallError::CopyOverlapping.into());
+                return;
+            }
+        }
 
         let invoke_context = question_mark!(
             self.invoke_context
@@ -4028,13 +4049,13 @@ mod tests {
 
     #[test]
     fn test_overlapping() {
-        assert!(!check_overlapping(10, 7, 3));
+        assert!(!check_overlapping_do_not_use(10, 7, 3));
-        assert!(check_overlapping(10, 8, 3));
+        assert!(check_overlapping_do_not_use(10, 8, 3));
-        assert!(check_overlapping(10, 9, 3));
+        assert!(check_overlapping_do_not_use(10, 9, 3));
-        assert!(check_overlapping(10, 10, 3));
+        assert!(check_overlapping_do_not_use(10, 10, 3));
-        assert!(check_overlapping(10, 11, 3));
+        assert!(check_overlapping_do_not_use(10, 11, 3));
-        assert!(check_overlapping(10, 12, 3));
+        assert!(check_overlapping_do_not_use(10, 12, 3));
-        assert!(!check_overlapping(10, 13, 3));
+        assert!(!check_overlapping_do_not_use(10, 13, 3));
     }
 
     fn call_program_address_common(

sdk/program/src/program_stubs.rs

@@ -54,7 +54,7 @@ pub trait SyscallStubs: Sync + Send {
     unsafe fn sol_memcpy(&self, dst: *mut u8, src: *const u8, n: usize) {
         // cannot be overlapping
         assert!(
-            !(dst as usize + n > src as usize && src as usize > dst as usize),
+            is_nonoverlapping(src as usize, dst as usize, n),
             "memcpy does not support overlapping regions"
         );
         std::ptr::copy_nonoverlapping(src, dst, n as usize);
@@ -193,3 +193,35 @@ pub(crate) fn sol_get_processed_sibling_instruction(index: usize) -> Option<Inst
 pub(crate) fn sol_get_stack_height() -> u64 {
     SYSCALL_STUBS.read().unwrap().sol_get_stack_height()
 }
+
+/// Check that two regions do not overlap.
+///
+/// Adapted from libcore, hidden to share with bpf_loader without being part of
+/// the API surface.
+#[doc(hidden)]
+pub fn is_nonoverlapping<N>(src: N, dst: N, count: N) -> bool
+where
+    N: Ord + std::ops::Sub<Output = N>,
+    <N as std::ops::Sub>::Output: Ord,
+{
+    let diff = if src > dst { src - dst } else { dst - src };
+    // If the absolute distance between the ptrs is at least as big as the size of the buffer,
+    // they do not overlap.
+    diff >= count
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_is_nonoverlapping() {
+        assert!(is_nonoverlapping(10, 7, 3));
+        assert!(!is_nonoverlapping(10, 8, 3));
+        assert!(!is_nonoverlapping(10, 9, 3));
+        assert!(!is_nonoverlapping(10, 10, 3));
+        assert!(!is_nonoverlapping(10, 11, 3));
+        assert!(!is_nonoverlapping(10, 12, 3));
+        assert!(is_nonoverlapping(10, 13, 3));
+    }
+}

sdk/src/feature_set.rs

@@ -303,6 +303,10 @@ pub mod add_get_processed_sibling_instruction_syscall {
     solana_sdk::declare_id!("CFK1hRCNy8JJuAAY8Pb2GjLFNdCThS2qwZNe3izzBMgn");
 }
 
+pub mod fixed_memcpy_nonoverlapping_check {
+    solana_sdk::declare_id!("36PRUK2Dz6HWYdG9SpjeAsF5F3KxnFCakA2BZMbtMhSb");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -373,6 +377,7 @@ lazy_static! {
         (disable_bpf_deprecated_load_instructions::id(), "Disable ldabs* and ldind* BPF instructions"),
         (disable_bpf_unresolved_symbols_at_runtime::id(), "Disable reporting of unresolved BPF symbols at runtime"),
         (add_get_processed_sibling_instruction_syscall::id(), "add add_get_processed_sibling_instruction_syscall"),
+        (fixed_memcpy_nonoverlapping_check::id(), "use correct check for nonoverlapping regions in memcpy syscall"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()