Re: [PATCH RESEND v2 2/2] migration/xbzrle: fix out-of-bounds write with axv512
Matheus Tavares Bernardino wrote: > xbzrle_encode_buffer_avx512() checks for overflows too scarcely in its > outer loop, causing out-of-bounds writes: > > $ ../configure --target-list=aarch64-softmmu --enable-sanitizers > --enable-avx512bw > $ make tests/unit/test-xbzrle && ./tests/unit/test-xbzrle > > ==5518==ERROR: AddressSanitizer: heap-buffer-overflow on address > 0x6210b100 at pc 0x561109a7714d bp 0x7ffed712a440 sp 0x7ffed712a430 > WRITE of size 1 at 0x6210b100 thread T0 > #0 0x561109a7714c in uleb128_encode_small ../util/cutils.c:831 > #1 0x561109b67f6a in xbzrle_encode_buffer_avx512 ../migration/xbzrle.c:275 > #2 0x5611099a7428 in test_encode_decode_overflow > ../tests/unit/test-xbzrle.c:153 > #3 0x7fb2fb65a58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d) > #4 0x7fb2fb65a333 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a333) > #5 0x7fb2fb65aa79 in g_test_run_suite > (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa79) > #6 0x7fb2fb65aa94 in g_test_run > (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa94) > #7 0x5611099a3a23 in main ../tests/unit/test-xbzrle.c:218 > #8 0x7fb2fa78c082 in __libc_start_main > (/lib/x86_64-linux-gnu/libc.so.6+0x24082) > #9 0x5611099a608d in _start (/qemu/build/tests/unit/test-xbzrle+0x28408d) > > 0x6210b100 is located 0 bytes to the right of 4096-byte region > [0x6210a100,0x6210b100) > allocated by thread T0 here: > #0 0x7fb2fb823a06 in __interceptor_calloc > ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153 > #1 0x7fb2fb637ef0 in g_malloc0 > (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57ef0) > > Fix that by performing the overflow check in the inner loop, instead. > > Signed-off-by: Matheus Tavares Bernardino Reviewed-by: Juan Quintela queued. As David said, we can still improve the code. thanks, Juan.
Re: [PATCH RESEND v2 2/2] migration/xbzrle: fix out-of-bounds write with axv512
* Matheus Tavares Bernardino (quic_mathb...@quicinc.com) wrote:
> xbzrle_encode_buffer_avx512() checks for overflows too scarcely in its
> outer loop, causing out-of-bounds writes:
>
> $ ../configure --target-list=aarch64-softmmu --enable-sanitizers
> --enable-avx512bw
> $ make tests/unit/test-xbzrle && ./tests/unit/test-xbzrle
>
> ==5518==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x6210b100 at pc 0x561109a7714d bp 0x7ffed712a440 sp 0x7ffed712a430
> WRITE of size 1 at 0x6210b100 thread T0
> #0 0x561109a7714c in uleb128_encode_small ../util/cutils.c:831
> #1 0x561109b67f6a in xbzrle_encode_buffer_avx512 ../migration/xbzrle.c:275
> #2 0x5611099a7428 in test_encode_decode_overflow
> ../tests/unit/test-xbzrle.c:153
> #3 0x7fb2fb65a58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
> #4 0x7fb2fb65a333 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a333)
> #5 0x7fb2fb65aa79 in g_test_run_suite
> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa79)
> #6 0x7fb2fb65aa94 in g_test_run
> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa94)
> #7 0x5611099a3a23 in main ../tests/unit/test-xbzrle.c:218
> #8 0x7fb2fa78c082 in __libc_start_main
> (/lib/x86_64-linux-gnu/libc.so.6+0x24082)
> #9 0x5611099a608d in _start (/qemu/build/tests/unit/test-xbzrle+0x28408d)
>
> 0x6210b100 is located 0 bytes to the right of 4096-byte region
> [0x6210a100,0x6210b100)
> allocated by thread T0 here:
> #0 0x7fb2fb823a06 in __interceptor_calloc
> ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
> #1 0x7fb2fb637ef0 in g_malloc0
> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57ef0)
>
> Fix that by performing the overflow check in the inner loop, instead.
>
> Signed-off-by: Matheus Tavares Bernardino
> ---
> migration/xbzrle.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/migration/xbzrle.c b/migration/xbzrle.c
> index 21b92d4eae..c6f8b20917 100644
> --- a/migration/xbzrle.c
> +++ b/migration/xbzrle.c
> @@ -197,10 +197,6 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf,
> uint8_t *new_buf, int slen,
> __m512i r = _mm512_set1_epi32(0);
>
> while (count512s) {
> -if (d + 2 > dlen) {
> -return -1;
> -}
> -
> int bytes_to_check = 64;
> uint64_t mask = 0x;
> if (count512s == 1) {
> @@ -216,6 +212,9 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t
> *new_buf, int slen,
>
> bool is_same = (comp & 0x1);
> while (bytes_to_check) {
> +if (d + 2 > dlen) {
> +return -1;
> +}
I agree that's better, so:
Reviewed-by: Dr. David Alan Gilbert
but is it sufficient?
In that bytes_to_check loop there are 4 calls to uleb128_encode_small
with another one just off the end of the loop.
I've not figured out all the legal combos, but I'm pretty sure at least
a few can trigger in one iteration - so don't we need those checks
before ecah call?
Dave
> if (is_same) {
> if (nzrun_len) {
> d += uleb128_encode_small(dst + d, nzrun_len);
> --
> 2.39.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PATCH RESEND v2 2/2] migration/xbzrle: fix out-of-bounds write with axv512
xbzrle_encode_buffer_avx512() checks for overflows too scarcely in its
outer loop, causing out-of-bounds writes:
$ ../configure --target-list=aarch64-softmmu --enable-sanitizers
--enable-avx512bw
$ make tests/unit/test-xbzrle && ./tests/unit/test-xbzrle
==5518==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6210b100
at pc 0x561109a7714d bp 0x7ffed712a440 sp 0x7ffed712a430
WRITE of size 1 at 0x6210b100 thread T0
#0 0x561109a7714c in uleb128_encode_small ../util/cutils.c:831
#1 0x561109b67f6a in xbzrle_encode_buffer_avx512 ../migration/xbzrle.c:275
#2 0x5611099a7428 in test_encode_decode_overflow
../tests/unit/test-xbzrle.c:153
#3 0x7fb2fb65a58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
#4 0x7fb2fb65a333 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a333)
#5 0x7fb2fb65aa79 in g_test_run_suite
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa79)
#6 0x7fb2fb65aa94 in g_test_run
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa94)
#7 0x5611099a3a23 in main ../tests/unit/test-xbzrle.c:218
#8 0x7fb2fa78c082 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x24082)
#9 0x5611099a608d in _start (/qemu/build/tests/unit/test-xbzrle+0x28408d)
0x6210b100 is located 0 bytes to the right of 4096-byte region
[0x6210a100,0x6210b100)
allocated by thread T0 here:
#0 0x7fb2fb823a06 in __interceptor_calloc
../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
#1 0x7fb2fb637ef0 in g_malloc0
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57ef0)
Fix that by performing the overflow check in the inner loop, instead.
Signed-off-by: Matheus Tavares Bernardino
---
migration/xbzrle.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/migration/xbzrle.c b/migration/xbzrle.c
index 21b92d4eae..c6f8b20917 100644
--- a/migration/xbzrle.c
+++ b/migration/xbzrle.c
@@ -197,10 +197,6 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t
*new_buf, int slen,
__m512i r = _mm512_set1_epi32(0);
while (count512s) {
-if (d + 2 > dlen) {
-return -1;
-}
-
int bytes_to_check = 64;
uint64_t mask = 0x;
if (count512s == 1) {
@@ -216,6 +212,9 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t
*new_buf, int slen,
bool is_same = (comp & 0x1);
while (bytes_to_check) {
+if (d + 2 > dlen) {
+return -1;
+}
if (is_same) {
if (nzrun_len) {
d += uleb128_encode_small(dst + d, nzrun_len);
--
2.39.1
