From c6afe9fbe52c5d8bb9051889aafea7f61063a6b9 Mon Sep 17 00:00:00 2001 From: ibhatt-jumptrading Date: Wed, 27 Nov 2024 15:56:33 +0000 Subject: [PATCH] flamenco: patch for oob memmove --- .../syscall-fixtures/memmove.list | 4 ++++ src/flamenco/vm/fd_vm_private.h | 20 ++++++++++++++++++- src/flamenco/vm/syscall/fd_vm_syscall_util.c | 12 ++++++----- 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/contrib/test/test-vectors-fixtures/syscall-fixtures/memmove.list b/contrib/test/test-vectors-fixtures/syscall-fixtures/memmove.list index f2cde68f69..9e0db0e4be 100644 --- a/contrib/test/test-vectors-fixtures/syscall-fixtures/memmove.list +++ b/contrib/test/test-vectors-fixtures/syscall-fixtures/memmove.list @@ -56,3 +56,7 @@ dump/test-vectors/syscall/fixtures/memmove/f0d9bc458582616f2c6c0e64d2393baf36983 dump/test-vectors/syscall/fixtures/memmove/f328a3d7474e6f07491536ac3762377303297f28_4138657.fix dump/test-vectors/syscall/fixtures/memmove/faab0f1d8f10c5ba7743ae26102723dc0cec917c_855179.fix dump/test-vectors/syscall/fixtures/memmove/ae20fed7fe60ee167563ff17f2c89df19962c118_4177560.fix +dump/test-vectors/syscall/fixtures/memmove/36dcdc2de61097fa7c76f409496a58ace0ce03bd_1979302.fix +dump/test-vectors/syscall/fixtures/memmove/55af68a28873e9f08db808a054cec8ac07f54f41_1927408.fix +dump/test-vectors/syscall/fixtures/memmove/d75da6e237385741c68d1722cffd33568c9bf50f_2012908.fix +dump/test-vectors/syscall/fixtures/memmove/e80a99f435c38a9f6153b2a2f4987d7d718a1e4b_1996804.fix diff --git a/src/flamenco/vm/fd_vm_private.h b/src/flamenco/vm/fd_vm_private.h index 48e136ad53..30f01501d3 100644 --- a/src/flamenco/vm/fd_vm_private.h +++ b/src/flamenco/vm/fd_vm_private.h @@ -550,7 +550,11 @@ static inline void fd_vm_mem_st_8( fd_vm_t const * vm, FD_VM_MEM_HADDR_ST_UNCHECKED has all of the checks of a load or a store, but intentionally omits the is_writable checks for the - input region that are done during memory translation. */ + input region that are done during memory translation. + + FD_VM_MEM_HADDR_ST_NO_SZ_CHECK does all of the checks of a load, + except for a check on the validity of the size of a load. It only + checks that the specific vaddr that is being translated is valid. */ #define FD_VM_MEM_HADDR_LD( vm, vaddr, align, sz ) (__extension__({ \ fd_vm_t const * _vm = (vm); \ @@ -581,6 +585,11 @@ static inline void fd_vm_mem_st_8( fd_vm_t const * vm, (void const *)_haddr; \ })) + +#define FD_VM_MEM_HADDR_LD_NO_SZ_CHECK( vm, vaddr, align ) (__extension__({ \ + FD_VM_MEM_HADDR_LD( vm, vaddr, align, 1UL ); \ + })) + static inline void * FD_VM_MEM_HADDR_ST_( fd_vm_t const *vm, ulong vaddr, ulong align, ulong sz, int *err ) { fd_vm_t const * _vm = (vm); @@ -644,6 +653,15 @@ FD_VM_MEM_HADDR_ST_( fd_vm_t const *vm, ulong vaddr, ulong align, ulong sz, int })) +#define FD_VM_MEM_HADDR_ST_NO_SZ_CHECK( vm, vaddr, align ) (__extension__({ \ + int _err = 0; \ + void * ret = FD_VM_MEM_HADDR_ST_( vm, vaddr, align, 1UL, &_err ); \ + if ( FD_UNLIKELY( 0 != _err )) \ + return _err; \ + ret; \ +})) + + #define FD_VM_MEM_HADDR_LD_FAST( vm, vaddr ) ((void const *)fd_vm_mem_haddr_fast( (vm), (vaddr), (vm)->region_haddr )) #define FD_VM_MEM_HADDR_ST_FAST( vm, vaddr ) ((void *)fd_vm_mem_haddr_fast( (vm), (vaddr), (vm)->region_haddr )) diff --git a/src/flamenco/vm/syscall/fd_vm_syscall_util.c b/src/flamenco/vm/syscall/fd_vm_syscall_util.c index b6cb6af008..b83e4ab95f 100644 --- a/src/flamenco/vm/syscall/fd_vm_syscall_util.c +++ b/src/flamenco/vm/syscall/fd_vm_syscall_util.c @@ -359,7 +359,7 @@ fd_vm_memmove( fd_vm_t * vm, vaddrs being in the input data region (since that is the only possible case you could have overlapping, chunked-up memmoves), Agave will iterate backwards in ANY region. If it eventually reaches the end of a region after iterating backwards and hits an access violation, the bytes from [region_begin, start_vaddr] will still be written to, causing fuzzing mismatches. - In this case, if we didn't have ther reverse flag, we would have thrown an access violation before any bytes were copied. + In this case, if we didn't have the reverse flag, we would have thrown an access violation before any bytes were copied. The same logic applies to memmoves that go past the high end of a region - reverse iteration logic would throw an access violation before any bytes were copied, while the current logic would copy the bytes until the end of the region. https://github.com/anza-xyz/agave/blob/v2.1.0/programs/bpf_loader/src/syscalls/mem_ops.rs#L184 */ @@ -391,12 +391,13 @@ fd_vm_memmove( fd_vm_t * vm, dst_bytes_rem_in_cur_region = fd_ulong_sat_sub( vm->input_mem_regions[ dst_region_idx ].region_sz, ( dst_offset - vm->input_mem_regions[ dst_region_idx ].vaddr_offset ) ); } } else { - dst_haddr = (uchar*)FD_VM_MEM_HADDR_ST_FAST( vm, dst_vaddr_begin ); + dst_haddr = (uchar*)FD_VM_MEM_HADDR_ST_NO_SZ_CHECK( vm, dst_vaddr_begin, FD_VM_ALIGN_RUST_U8 ); if( FD_UNLIKELY( reverse ) ) { - /* Bytes remaining is minimum of the offset from the beginning of the current region (+1 for inclusive region beginning) - and the number of storable bytes in the region. */ + /* Bytes remaining is minimum of the offset from the beginning of the current + region (+1 for inclusive region beginning) and the number of storable bytes in the region. */ dst_bytes_rem_in_cur_region = fd_ulong_min( vm->region_st_sz[ dst_region ], dst_offset + 1UL ); + } else { /* Bytes remaining is the number of writable bytes left in the region */ dst_bytes_rem_in_cur_region = fd_ulong_sat_sub( vm->region_st_sz[ dst_region ], dst_offset ); @@ -418,10 +419,11 @@ fd_vm_memmove( fd_vm_t * vm, src_bytes_rem_in_cur_region = fd_ulong_sat_sub( vm->input_mem_regions[ src_region_idx ].region_sz, ( src_offset - vm->input_mem_regions[ src_region_idx ].vaddr_offset ) ); } } else { - src_haddr = (uchar*)FD_VM_MEM_HADDR_LD_FAST( vm, src_vaddr_begin ); + src_haddr = (uchar*)FD_VM_MEM_HADDR_LD_NO_SZ_CHECK( vm, src_vaddr_begin, FD_VM_ALIGN_RUST_U8 ); if( FD_UNLIKELY( reverse ) ) { src_bytes_rem_in_cur_region = fd_ulong_min( vm->region_ld_sz[ src_region ], src_offset + 1UL ); + } else { src_bytes_rem_in_cur_region = fd_ulong_sat_sub( vm->region_ld_sz[ src_region ], src_offset ); }