1. Patchew
  2. QEMU
  3. View series

[PATCH] tests/qtest: fix heap-use-after-free

Dmitry Frolov posted 1 patch 1 week, 5 days ago 
tests/qtest/qos-test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] tests/qtest: fix heap-use-after-free
Posted by Dmitry Frolov 1 week, 5 days ago 
"int main(int argc, char **argv, char** envp)" is non-standart
Microsoft`s extention of the C language and it`s not portable.
In my particular case (Debian 13, clang-16) this raises wild-pointer
dereference with ASAN message "heap-use-after-free".

Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
 tests/qtest/qos-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 114f6bef27..e8ac00f0f7 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -326,7 +326,7 @@ static void walk_path(QOSGraphNode *orig_path, int len)
  *   machine/drivers/test objects
  * - Cleans up everything
  */
-int main(int argc, char **argv, char** envp)
+int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
@@ -336,7 +336,7 @@ int main(int argc, char **argv, char** envp)
 
     if (g_test_verbose()) {
         qos_printf("ENVIRONMENT VARIABLES: {\n");
-        for (char **env = envp; *env != 0; env++) {
+        for (char **env = environ; *env != 0; env++) {
             qos_printf("\t%s\n", *env);
         }
         qos_printf("}\n");
-- 
2.43.0
Re: [PATCH] tests/qtest: fix heap-use-after-free
Posted by Prasad Pandit 1 week, 5 days ago 
On Mon, 11 Nov 2024 at 14:37, Dmitry Frolov <frolov@swemel.ru> wrote:
> "int main(int argc, char **argv, char** envp)" is non-standart
> Microsoft`s extention of the C language and it`s not portable.
> In my particular case (Debian 13, clang-16) this raises wild-pointer
> dereference with ASAN message "heap-use-after-free".
...
>          qos_printf("ENVIRONMENT VARIABLES: {\n");
> -        for (char **env = envp; *env != 0; env++) {
> +        for (char **env = environ; *env != 0; env++) {
>              qos_printf("\t%s\n", *env);
>          }

* For heap-use-after-free, there needs to be a free(*env) call
somewhere. In the 'tests/qtest/qos-test.c' file, I couldn't see
environment variables being free'd anywhere. Above loop is only
printing them. Following small test.c did not reproduce the
'heap-use-after-free' error.
===
#include <stdio.h>
int
main(int argc, char *argv[], char **envp)
{
    int n = 0;
    for (char **p = envp; *p != 0; p++) {
        printf("environ[%d]: %s\n", n++, *p);
    }
    return 0;
}
$ cc -xc -o test test.c -lasan
===

* While the patch is okay, it is not clear why it fixes the
wild-pointer dereference and "heap-use-after-free" errors.

Thank you.
---
  - Prasad
Re: [PATCH] tests/qtest: fix heap-use-after-free
Posted by Дмитрий Фролов 1 week, 5 days ago 

On 11.11.2024 14:47, Prasad Pandit wrote:
> On Mon, 11 Nov 2024 at 14:37, Dmitry Frolov <frolov@swemel.ru> wrote:
>> "int main(int argc, char **argv, char** envp)" is non-standart
>> Microsoft`s extention of the C language and it`s not portable.
>> In my particular case (Debian 13, clang-16) this raises wild-pointer
>> dereference with ASAN message "heap-use-after-free".
> ...
>>           qos_printf("ENVIRONMENT VARIABLES: {\n");
>> -        for (char **env = envp; *env != 0; env++) {
>> +        for (char **env = environ; *env != 0; env++) {
>>               qos_printf("\t%s\n", *env);
>>           }
> * For heap-use-after-free, there needs to be a free(*env) call
> somewhere. In the 'tests/qtest/qos-test.c' file, I couldn't see
> environment variables being free'd anywhere. Above loop is only
> printing them.
Above loop dereferences the pointer env, which is pointing to
the memory area, which is not allowed to read.

>   Following small test.c did not reproduce the
> 'heap-use-after-free' error.
> ===
> #include <stdio.h>
> int
> main(int argc, char *argv[], char **envp)
> {
>      int n = 0;
>      for (char **p = envp; *p != 0; p++) {
>          printf("environ[%d]: %s\n", n++, *p);
>      }
>      return 0;
> }
> $ cc -xc -o test test.c -lasan
> ===
>
> * While the patch is okay, it is not clear why it fixes the
> wild-pointer dereference and "heap-use-after-free" errors.
>
> Thank you.
> ---
>    - Prasad
>
This example will work everywhere, where env pointer is
supported by OS/compiler/etc... Nevertheless, I am pointing on 2 facts:
1. "env" is Microsoft`s extension, not a standard
2. There is exact example, where standards violation raises
undefined behavior: debian13/clang16
Re: [PATCH] tests/qtest: fix heap-use-after-free
Posted by Prasad Pandit 1 week, 5 days ago 
On Mon, 11 Nov 2024 at 17:41, Дмитрий Фролов <frolov@swemel.ru> wrote:
> Above loop dereferences the pointer env, which is pointing to
> the memory area, which is not allowed to read.

* Not allowed to read environment variables? Is it because
Debian/clang does not support the '**envp' parameter? Is '**envp' set
to NULL on Debian? If '**envp' is not supported, then the compiler
should throw an error at build time, no?

> I am pointing on 2 facts:
> 1. "env" is Microsoft`s extension, not a standard
> 2. There is exact example, where standards violation raises
> undefined behavior: debian13/clang16
>

* If this is about Debian not supporting '**envp' parameter, then
it'll help if the commit message says "...Debian does not support this
non-standard extension and crashes QEMU". The asan error makes it
sound like the patch fixes the use-after-free issue. What happens if
the -lasan is not used? Does it still crash QEMUt?

Thank you.
---
  - Prasad
Re: [PATCH] tests/qtest: fix heap-use-after-free
Posted by Дмитрий Фролов 1 week, 5 days ago 

On 11.11.2024 15:51, Prasad Pandit wrote:
> On Mon, 11 Nov 2024 at 17:41, Дмитрий Фролов<frolov@swemel.ru>  wrote:
>> Above loop dereferences the pointer env, which is pointing to
>> the memory area, which is not allowed to read.
> * Not allowed to read environment variables? Is it because
> Debian/clang does not support the '**envp' parameter? Is '**envp' set
> to NULL on Debian? If '**envp' is not supported, then the compiler
> should throw an error at build time, no?
Not allowed to read the exact memory area, because it is marked as freed.
>> I am pointing on 2 facts:
>> 1. "env" is Microsoft`s extension, not a standard
>> 2. There is exact example, where standards violation raises
>> undefined behavior: debian13/clang16
>>
> * If this is about Debian not supporting '**envp' parameter, then
> it'll help if the commit message says "...Debian does not support this
> non-standard extension and crashes QEMU".
Since this is UB, it does not matter, if a crash happens or not.
ASAN just helps to highlight the hidden problem.

> The asan error makes it
> sound like the patch fixes the use-after-free issue.
I didn`t want to confuse anybody, but this is exactly,
what is actually happening (see log below).

> What happens if
> the -lasan is not used? Does it still crash QEMUt?
>
> Thank you.
> ---
>    - Prasad
>
When saintizers are disabled, qos-test passes successfully.
qos-test fails when qemu is built with enabled sanitizers
(--enable-asan --enable-ubsan)

==879133==ERROR: AddressSanitizer: heap-use-after-free on address 
0x514000000040 at pc 0x55eae79b407c bp 0x7ffd028715d0 sp 0x7ffd028715c8
READ of size 8 at 0x514000000040 thread T0
     #0 0x55eae79b407b in main 
/home/df/projects/upstream/qemu/build/../tests/qtest/qos-test.c:339:33
     #1 0x7f9011760c89  (/lib/x86_64-linux-gnu/libc.so.6+0x27c89) 
(BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38)
     #2 0x7f9011760d44 in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x27d44) (BuildId: 
61cf5c68463ab7677fa14f071a036eda24d0cc38)
     #3 0x55eae77a5c60 in _start 
(/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x209c60) 
(BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e)

0x514000000040 is located 0 bytes inside of 416-byte region 
[0x514000000040,0x5140000001e0)
freed by thread T0 here:
     #0 0x55eae7840ce9 in __interceptor_realloc 
(/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x2a4ce9) 
(BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e)
     #1 0x7f901177b596  (/lib/x86_64-linux-gnu/libc.so.6+0x42596) 
(BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38)

previously allocated by thread T0 here:
     #0 0x55eae7840ce9 in __interceptor_realloc 
(/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x2a4ce9) 
(BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e)
     #1 0x7f901177b596  (/lib/x86_64-linux-gnu/libc.so.6+0x42596) 
(BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38)

SUMMARY: AddressSanitizer: heap-use-after-free 
/home/df/projects/upstream/qemu/build/../tests/qtest/qos-test.c:339:33 
in main
Shadow bytes around the buggy address:
   0x513ffffffd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
   0x513ffffffe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
   0x513ffffffe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
   0x513fffffff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
   0x513fffffff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x514000000000: fa fa fa fa fa fa fa fa[fd]fd fd fd fd fd fd fd
   0x514000000080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
   0x514000000100: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
   0x514000000180: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
   0x514000000200: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
   0x514000000280: 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
==879133==ABORTING
Aborted


With best regards,
Dmitry.
Re: [sdl-qemu] [PATCH] tests/qtest: fix heap-use-after-free
Posted by Alexey Khoroshilov 1 week, 4 days ago 
On 11.11.2024 16:35, Дмитрий Фролов wrote:
>
>
> On 11.11.2024 15:51, Prasad Pandit wrote:
>> On Mon, 11 Nov 2024 at 17:41, Дмитрий Фролов <frolov@swemel.ru> wrote:
>>> Above loop dereferences the pointer env, which is pointing to
>>> the memory area, which is not allowed to read.
>> * Not allowed to read environment variables? Is it because
>> Debian/clang does not support the '**envp' parameter? Is '**envp' set
>> to NULL on Debian? If '**envp' is not supported, then the compiler
>> should throw an error at build time, no?
> Not allowed to read the exact memory area, because it is marked as freed.
As far as I understand, heap-use-after-free means a situation when code
allocates memory then frees it and then access it.
Here the code just accesses invalid memory because of nonstandard main()
call convention.

If it is correct, the patch title could be "tests/qtest: make access to
environment variables portable"

--
Alexey

>>> I am pointing on 2 facts:
>>> 1. "env" is Microsoft`s extension, not a standard
>>> 2. There is exact example, where standards violation raises
>>> undefined behavior: debian13/clang16
>>>
>> * If this is about Debian not supporting '**envp' parameter, then
>> it'll help if the commit message says "...Debian does not support this
>> non-standard extension and crashes QEMU". 
> Since this is UB, it does not matter, if a crash happens or not.
> ASAN just helps to highlight the hidden problem.
>  
>> The asan error makes it
>> sound like the patch fixes the use-after-free issue. 
> I didn`t want to confuse anybody, but this is exactly,
> what is actually happening (see log below).
>
>> What happens if
>> the -lasan is not used? Does it still crash QEMUt?
>>
>> Thank you.
>> ---
>>   - Prasad
>>
> When saintizers are disabled, qos-test passes successfully.
> qos-test fails when qemu is built with enabled sanitizers
> (--enable-asan --enable-ubsan)
>
> ==879133==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x514000000040 at pc 0x55eae79b407c bp 0x7ffd028715d0 sp 0x7ffd028715c8
> READ of size 8 at 0x514000000040 thread T0
>     #0 0x55eae79b407b in main
> /home/df/projects/upstream/qemu/build/../tests/qtest/qos-test.c:339:33
>     #1 0x7f9011760c89  (/lib/x86_64-linux-gnu/libc.so.6+0x27c89)
> (BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38)
>     #2 0x7f9011760d44 in __libc_start_main
> (/lib/x86_64-linux-gnu/libc.so.6+0x27d44) (BuildId:
> 61cf5c68463ab7677fa14f071a036eda24d0cc38)
>     #3 0x55eae77a5c60 in _start
> (/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x209c60)
> (BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e)
>
> 0x514000000040 is located 0 bytes inside of 416-byte region
> [0x514000000040,0x5140000001e0)
> freed by thread T0 here:
>     #0 0x55eae7840ce9 in __interceptor_realloc
> (/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x2a4ce9)
> (BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e)
>     #1 0x7f901177b596  (/lib/x86_64-linux-gnu/libc.so.6+0x42596)
> (BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38)
>
> previously allocated by thread T0 here:
>     #0 0x55eae7840ce9 in __interceptor_realloc
> (/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x2a4ce9)
> (BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e)
>     #1 0x7f901177b596  (/lib/x86_64-linux-gnu/libc.so.6+0x42596)
> (BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38)
>
> SUMMARY: AddressSanitizer: heap-use-after-free
> /home/df/projects/upstream/qemu/build/../tests/qtest/qos-test.c:339:33
> in main
> Shadow bytes around the buggy address:
>   0x513ffffffd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x513ffffffe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x513ffffffe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x513fffffff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x513fffffff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> =>0x514000000000: fa fa fa fa fa fa fa fa[fd]fd fd fd fd fd fd fd
>   0x514000000080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>   0x514000000100: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>   0x514000000180: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
>   0x514000000200: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
>   0x514000000280: 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
> ==879133==ABORTING
> Aborted
>
>
> With best regards,
> Dmitry.
>
> _______________________________________________
> sdl.qemu mailing list
> sdl.qemu@linuxtesting.org
> http://linuxtesting.org/cgi-bin/mailman/listinfo/sdl.qemu
Re: [sdl-qemu] [PATCH] tests/qtest: fix heap-use-after-free
Posted by Prasad Pandit 1 week, 4 days ago 
Hi,

On Mon, 11 Nov 2024 at 22:51, Alexey Khoroshilov <khoroshilov@ispras.ru> wrote:
> On 11.11.2024 16:35, Дмитрий Фролов wrote:
> Not allowed to read the exact memory area, because it is marked as freed.
>
> As far as I understand, heap-use-after-free means a situation when code allocates memory then frees it and then access it.
> Here the code just accesses invalid memory because of nonstandard main() call convention.
>
> If it is correct, the patch title could be "tests/qtest: make access to environment variables portable"
...
> Since this is UB, it does not matter, if a crash happens or not.
> ASAN just helps to highlight the hidden problem.

* It is not clear what is happening here.  The third parameter
(**envp) looks widely supported.

    -> https://www.gnu.org/software/libc/manual/html_node/Program-Arguments.html

Above document says and following program confirms '**envp' points to
the same address as '*environ' variable. It also says '**envp' is not
portable.
===
#define _GNU_SOURCE
#include <stdio.h>
#include <unistd.h>

int main(int argc, char *argv[], char **envp)
{
    printf("environ: %p, envp: %p\n", environ, envp);
    return 0;
}
$ cc -xc -o test test.c -lasan
$ ./test
environ: 0x7ffc5eb12168, envp: 0x7ffc5eb12168
===

> When saintizers are disabled, qos-test passes successfully.
> qos-test fails when qemu is built with enabled sanitizers

* That means Debian/clang has no qualms about the third parameter. It
is not a portability issue then. This Debian page also indicates usage
of '**envp' parameter ->
https://www.debian.org/doc/manuals/debian-reference/ch12.en.html.

* If both '*environ' and '**envp' point to the same address, why does
ASAN throw error with one and not with the other? Where is that memory
getting free'd?

* The patch looks fairly innocuous, but along with the commit message
it is confusing enough to review it. I'd be okay to review it if we
get some clarity about what is going on there.

Thank you.
---
  - Prasad

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.