From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53920) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eX8Zf-0005fP-Qj for qemu-devel@nongnu.org; Thu, 04 Jan 2018 11:41:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eX8Ze-0007QA-GM for qemu-devel@nongnu.org; Thu, 04 Jan 2018 11:40:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37022) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eX8Ze-0007OF-7d for qemu-devel@nongnu.org; Thu, 04 Jan 2018 11:40:58 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 65DBC629C6 for ; Thu, 4 Jan 2018 16:40:57 +0000 (UTC) References: <20180104160523.22995-1-marcandre.lureau@redhat.com> <20180104160523.22995-13-marcandre.lureau@redhat.com> From: Thomas Huth Message-ID: <3c07180c-f65c-fdcf-fb53-2369f6af5cf6@redhat.com> Date: Thu, 4 Jan 2018 17:40:36 +0100 MIME-Version: 1.0 In-Reply-To: <20180104160523.22995-13-marcandre.lureau@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 12/18] crypto: fix stack-buffer-overflow error List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, famz@redhat.com On 04.01.2018 17:05, Marc-Andr=C3=A9 Lureau wrote: > ASAN complains about: >=20 > =3D=3D8856=3D=3DERROR: AddressSanitizer: stack-buffer-overflow on addre= ss 0x7ffd8a1fe168 at pc 0x561136cb4451 bp 0x7ffd8a1fe130 sp 0x7ffd8a1fd8e= 0 > READ of size 16 at 0x7ffd8a1fe168 thread T0 > #0 0x561136cb4450 in __asan_memcpy (/home/elmarco/src/qq/build/test= s/test-crypto-ivgen+0x110450) > #1 0x561136d2a6a7 in qcrypto_ivgen_essiv_calculate /home/elmarco/sr= c/qq/crypto/ivgen-essiv.c:83:5 > #2 0x561136d29af8 in qcrypto_ivgen_calculate /home/elmarco/src/qq/c= rypto/ivgen.c:72:12 > #3 0x561136d07c8e in test_ivgen /home/elmarco/src/qq/tests/test-cry= pto-ivgen.c:148:5 > #4 0x7f77772c3b04 in test_case_run /home/elmarco/src/gnome/glib/bui= lddir/../glib/gtestutils.c:2237 > #5 0x7f77772c3ec4 in g_test_run_suite_internal /home/elmarco/src/gn= ome/glib/builddir/../glib/gtestutils.c:2321 > #6 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gn= ome/glib/builddir/../glib/gtestutils.c:2333 > #7 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gn= ome/glib/builddir/../glib/gtestutils.c:2333 > #8 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gn= ome/glib/builddir/../glib/gtestutils.c:2333 > #9 0x7f77772c4184 in g_test_run_suite /home/elmarco/src/gnome/glib/= builddir/../glib/gtestutils.c:2408 > #10 0x7f77772c2e0d in g_test_run /home/elmarco/src/gnome/glib/build= dir/../glib/gtestutils.c:1674 > #11 0x561136d0799b in main /home/elmarco/src/qq/tests/test-crypto-i= vgen.c:173:12 > #12 0x7f77756e6039 in __libc_start_main (/lib64/libc.so.6+0x21039) > #13 0x561136c13d89 in _start (/home/elmarco/src/qq/build/tests/test= -crypto-ivgen+0x6fd89) >=20 > Address 0x7ffd8a1fe168 is located in stack of thread T0 at offset 40 in= frame > #0 0x561136d2a40f in qcrypto_ivgen_essiv_calculate /home/elmarco/sr= c/qq/crypto/ivgen-essiv.c:76 >=20 > This frame has 1 object(s): > [32, 40) 'sector.addr' <=3D=3D Memory access at offset 40 overflows= this variable > HINT: this may be a false positive if your program uses some custom sta= ck unwind mechanism or swapcontext > (longjmp and C++ exceptions *are* supported) > SUMMARY: AddressSanitizer: stack-buffer-overflow (/home/elmarco/src/qq/= build/tests/test-crypto-ivgen+0x110450) in __asan_memcpy > Shadow bytes around the buggy address: > 0x100031437bd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x100031437be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x100031437bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x100031437c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x100031437c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > =3D>0x100031437c20: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00[f3]f3 f3 > 0x100031437c30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x100031437c40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x100031437c50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x100031437c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x100031437c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > Shadow byte legend (one shadow byte represents 8 application bytes): > Addressable: 00 > Partially addressable: 01 02 03 04 05 06 07 > Heap left redzone: fa > Freed heap region: fd > Stack left redzone: f1 > Stack mid redzone: f2 > Stack right redzone: f3 > Stack after return: f5 > Stack use after scope: f8 > Global redzone: f9 > Global init order: f6 > Poisoned by user: f7 > Container overflow: fc > Array cookie: ac > Intra object redzone: bb > ASan internal: fe > Left alloca redzone: ca > Right alloca redzone: cb >=20 > It looks like the rest of the code copes with ndata being larger than > sizeof(sector), so limit the memcpy() range. >=20 > Signed-off-by: Marc-Andr=C3=A9 Lureau > Reviewed-by: Daniel P. Berrange > --- > crypto/ivgen-essiv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) >=20 > diff --git a/crypto/ivgen-essiv.c b/crypto/ivgen-essiv.c > index cba20bde6c..ad4d926c19 100644 > --- a/crypto/ivgen-essiv.c > +++ b/crypto/ivgen-essiv.c > @@ -79,7 +79,7 @@ static int qcrypto_ivgen_essiv_calculate(QCryptoIVGen= *ivgen, > uint8_t *data =3D g_new(uint8_t, ndata); > =20 > sector =3D cpu_to_le64(sector); > - memcpy(data, (uint8_t *)§or, ndata); > + memcpy(data, (uint8_t *)§or, MIN(sizeof(sector), ndata)); > if (sizeof(sector) < ndata) { > memset(data + sizeof(sector), 0, ndata - sizeof(sector)); > } >=20 Ah, funny, completely unaware of your patch series, I accidentally came to the same conclusion two days ago: https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg00062.html So if you like, feel free to add: Reviewed-by: Thomas Huth Tested-by: Thomas Huth