When the length of mname is less than 5, memcpy ("xenfv", mname, 5) will cause
heap buffer overflow. Therefore, use strcmp to avoid this problem.

The asan showed stack:

ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000f2f4 at
pc 0x7f65d8cc2225 bp 0x7ffe93cc5a60 sp 0x7ffe93cc5208 READ of size 5 at
0x60200000f2f4 thread T0
    #0 0x7f65d8cc2224 in memcmp (/lib64/libasan.so.5+0xdf224)
    #1 0x5632c20be95b in qtest_cb_for_every_machine tests/qtest/libqtest.c:1282
    #2 0x5632c20b7995 in main tests/qtest/test-hmp.c:160
    #3 0x7f65d88fed42 in __libc_start_main (/lib64/libc.so.6+0x26d42)
    #4 0x5632c20b72cd in _start (build/tests/qtest/test-hmp+0x542cd)

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Thomas Huth <thuth@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>
---
 tests/qtest/libqtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index e49f3a1e45..e8179a3509 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1281,7 +1281,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
         g_assert(qstr);
         mname = qstring_get_str(qstr);
         /* Ignore machines that cannot be used for qtests */
-        if (!memcmp("xenfv", mname, 5) || g_str_equal("xenpv", mname)) {
+        if (!strcmp("xenfv", mname) || g_str_equal("xenpv", mname)) {
             continue;
         }
         if (!skip_old_versioned || !qtest_is_old_versioned_machine(mname)) {
-- 
2.23.0

On 04/01/2021 15.10, Gan Qixin wrote:
> When the length of mname is less than 5, memcpy ("xenfv", mname, 5) will cause
> heap buffer overflow. Therefore, use strcmp to avoid this problem.
> 
> The asan showed stack:
> 
> ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000f2f4 at
> pc 0x7f65d8cc2225 bp 0x7ffe93cc5a60 sp 0x7ffe93cc5208 READ of size 5 at
> 0x60200000f2f4 thread T0
>      #0 0x7f65d8cc2224 in memcmp (/lib64/libasan.so.5+0xdf224)
>      #1 0x5632c20be95b in qtest_cb_for_every_machine tests/qtest/libqtest.c:1282
>      #2 0x5632c20b7995 in main tests/qtest/test-hmp.c:160
>      #3 0x7f65d88fed42 in __libc_start_main (/lib64/libc.so.6+0x26d42)
>      #4 0x5632c20b72cd in _start (build/tests/qtest/test-hmp+0x542cd)
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> ---
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Laurent Vivier <lvivier@redhat.com>
> ---
>   tests/qtest/libqtest.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index e49f3a1e45..e8179a3509 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -1281,7 +1281,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
>           g_assert(qstr);
>           mname = qstring_get_str(qstr);
>           /* Ignore machines that cannot be used for qtests */
> -        if (!memcmp("xenfv", mname, 5) || g_str_equal("xenpv", mname)) {
> +        if (!strcmp("xenfv", mname) || g_str_equal("xenpv", mname)) {

Using strcmp() is likely wrong here, since we're talking about strings like 
"xenfv-4.2" here ... so I guess strncmp(..., 5) would be the right way to go?

  Thomas

> From: Thomas Huth [mailto:thuth@redhat.com]
> Sent: Tuesday, January 5, 2021 5:47 PM
> To: ganqixin <ganqixin@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; Euler Robot
> <euler.robot@huawei.com>; Laurent Vivier <lvivier@redhat.com>
> Subject: Re: [PATCH] qtest/libqtest.c: fix heap-buffer-overflow in
> qtest_cb_for_every_machine()
> 
> On 04/01/2021 15.10, Gan Qixin wrote:
> > When the length of mname is less than 5, memcpy ("xenfv", mname, 5)
> > will cause heap buffer overflow. Therefore, use strcmp to avoid this
> problem.
> >
> > The asan showed stack:
> >
> > ERROR: AddressSanitizer: heap-buffer-overflow on address
> > 0x60200000f2f4 at pc 0x7f65d8cc2225 bp 0x7ffe93cc5a60 sp
> > 0x7ffe93cc5208 READ of size 5 at
> > 0x60200000f2f4 thread T0
> >      #0 0x7f65d8cc2224 in memcmp (/lib64/libasan.so.5+0xdf224)
> >      #1 0x5632c20be95b in qtest_cb_for_every_machine
> tests/qtest/libqtest.c:1282
> >      #2 0x5632c20b7995 in main tests/qtest/test-hmp.c:160
> >      #3 0x7f65d88fed42 in __libc_start_main (/lib64/libc.so.6+0x26d42)
> >      #4 0x5632c20b72cd in _start (build/tests/qtest/test-hmp+0x542cd)
> >
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> > ---
> > Cc: Thomas Huth <thuth@redhat.com>
> > Cc: Laurent Vivier <lvivier@redhat.com>
> > ---
> >   tests/qtest/libqtest.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index
> > e49f3a1e45..e8179a3509 100644
> > --- a/tests/qtest/libqtest.c
> > +++ b/tests/qtest/libqtest.c
> > @@ -1281,7 +1281,7 @@ void qtest_cb_for_every_machine(void
> (*cb)(const char *machine),
> >           g_assert(qstr);
> >           mname = qstring_get_str(qstr);
> >           /* Ignore machines that cannot be used for qtests */
> > -        if (!memcmp("xenfv", mname, 5) || g_str_equal("xenpv",
> mname)) {
> > +        if (!strcmp("xenfv", mname) || g_str_equal("xenpv", mname)) {
> 
> Using strcmp() is likely wrong here, since we're talking about strings like
> "xenfv-4.2" here ... so I guess strncmp(..., 5) would be the right way to go?


Yes, using strcmp() is wrong, I will modify this patch. Thanks for your reply!