Skip to content

Commit

Permalink
flamenco: patch for oob memmove
Browse files Browse the repository at this point in the history
  • Loading branch information
ibhatt-jumptrading committed Nov 27, 2024
1 parent a3c213f commit c6afe9f
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
20 changes: 19 additions & 1 deletion src/flamenco/vm/fd_vm_private.h
Original file line number Diff line number Diff line change
Expand Up @@ -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); \
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 ))

Expand Down
12 changes: 7 additions & 5 deletions src/flamenco/vm/syscall/fd_vm_syscall_util.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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 );
Expand All @@ -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 );
}
Expand Down

0 comments on commit c6afe9f

Please sign in to comment.