Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upx-4.0.2-armeb_linux/upx hangs when running under qemu-armeb #687

Closed
markus-oberhumer opened this issue Jul 19, 2023 · 47 comments
Closed
Labels
notabug The issue is not a bug. os:Linux

Comments

@markus-oberhumer
Copy link
Collaborator

markus-oberhumer commented Jul 19, 2023

UPDATE 1: this is NOT a UPX bug; qemu-armeb seems broken since qemu >= 7.0.0; qemu-6.2.0 and earlier do work

UPDATE 2: the UPX team believes this is a qemu bug (regression) introduced between qemu-6.2 and qemu-7.0, probably by commit https://gitlab.com/qemu-project/qemu/-/commit/7f4f0d9ea87097a31b36bcc52b7368efed35593c

UPDATE 3: the problem has been identified and fixed, and hopefully will be resolved in the next qemu release

UPDATE 4: qemu-8.1.0-rc2 includes the fixes; back-ports to older stable qemu versions are pending

UPDATE 5: also fixed in qemu-8.0.4 and qemu-7.2.5

What's the problem (or question)?

qemu-armeb upx-4.0.2-armeb_linux/upx hangs when throwing a C++ exception, with 100% CPU usage

qemu-7.1.0 (Alpine Linux 3.17) HANGS
qemu-7.2.1 (Fedora 38) HANGS
qemu-8 git HEAD with upx-fix HANGS

Earlier QEMU versions (qemu-6 and below) seem to work, so this might be a QEMU regression.

How can we reproduce the issue?

Download and untar https://github.com/upx/upx/releases/download/v4.0.2/upx-4.0.2-armeb_linux.tar.xz

# (assuming that the file "xxx" does not exist)

$ qemu-armeb ./upx-4.0.2-armeb_linux/upx xxx
[...]
upx: xxx: FileNotFoundException: xxx: No such file or directory

[NOW HANGS with 100% CPU usage]

Note that this problem is probably unrelated to UPX as the unpacked executable also hangs.

Hint: see https://github.com/upx/upx/blob/devel/misc/test-qemu-with-podman for some simple support scripts how to test various QEMU versions under Podman

P.S. I stumbled on this while testing the fix for #683

@markus-oberhumer
Copy link
Collaborator Author

@jreiser Could you please run a quick test and confirm the bug? Thanks!

@jreiser
Copy link
Collaborator

jreiser commented Jul 19, 2023

@markus-oberhumer I will wait until tomorow in hope that the fog of too many proposed patches and actual patches and Pull requests clears from qemu.

@markus-oberhumer
Copy link
Collaborator Author

CC @hdeller Hi Helge, sorry for bothering you again, but maybe you want to have a look. Or otherwise please connect us to the relevant QEMU team. Thanks!

@jreiser
Copy link
Collaborator

jreiser commented Jul 20, 2023

commit 2c27fdc7a626408ee2cf30d791aa0b63027c7404 (HEAD -> master, tag: v8.1.0-rc0, origin/master, origin/HEAD)
Date:   Wed Jul 19 20:31:43 2023 +0100

$ gdb --args qemu/build/qemu-armeb -strace upx-4.0.2-armeb_linux/upx this-file-does-not-exist
649735 open("/proc/self/exe",O_RDONLY) = 3
649735 mmap2(NULL,2728,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS,-1,0) = 0x40802000
649735 mprotect(0x40802000,2728,PROT_EXEC|PROT_READ) = 0
649735 readlink("/proc/self/exe",0x407ff1e0,4095) = 62
649735 munmap(0x00010000,2082700) = 0
649735 mmap2(0x00010000,1998812,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00010000
649735 mprotect(0x00010000,1998812,PROT_EXEC|PROT_READ) = 0
649735 mmap2(0x00208000,5036,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00208000
649735 mprotect(0x00208000,5036,PROT_READ|PROT_WRITE) = 0
649735 mmap2(0x0020a000,10124,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x0020a000
649735 brk(0x0020c78c) = 0x0020c78c

Thread 1 "qemu-armeb" received signal SIGSEGV, Segmentation fault.
0x00007fffe800f12e in code_gen_buffer ()
(gdb) x/i $pc
=> 0x7fffe800f12e <code_gen_buffer+61697>:	movbe  %gs:(%r12),%r12d

so upx-fix has not propagated into qemu-v8.1.0-rc0.

Try another version, this time of Helga's qemu-hppa with upx-fix:

commit 29b6a3dcb8d49d89dedff308f645ea058c39fb98 (HEAD -> upx-fix, origin/upx-fix)
Date:   Mon Jul 17 12:39:38 2023 +0200

$ gdb --args qemu-hppa/build/qemu-armeb -strace upx-4.0.2-armeb_linux/upx this-file-does-not-exist
650645 open("/proc/self/exe",O_RDONLY) = 3
650645 mmap2(NULL,2728,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS,-1,0) = 0x40802000
650645 mprotect(0x40802000,2728,PROT_EXEC|PROT_READ) = 0
650645 readlink("/proc/self/exe",0x407ff200,4095) = 62
650645 munmap(0x00010000,2082700) = 0
650645 mmap2(0x00010000,1998812,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00010000
650645 mprotect(0x00010000,1998812,PROT_EXEC|PROT_READ) = 0
650645 mmap2(0x00208000,5036,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x00208000
650645 mprotect(0x00208000,5036,PROT_READ|PROT_WRITE) = 0
650645 mmap2(0x0020a000,10124,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED,-1,0) = 0x0020a000
650645 brk(0x0020c78c) = 0x0029591b
650645 munmap(0x00210000,2097152) = 0
650645 mmap2(NULL,4096,PROT_READ,MAP_PRIVATE,3,0) = 0x40803000
650645 close(3) = 0
650645 munmap(0x40802000,2728) = 0
650645 set_tid_address(0x20c788) = 650645
650645 brk(NULL) = 0x0029591b
650645 brk(0x0029b000) = 0x0029b000
650645 clock_gettime64(CLOCK_PROCESS_CPUTIME_ID,0x407fffa8) = 0 ({tv_sec=0,tv_nsec=43373859})
650645 ioctl(0,TIOCGWINSZ,0x407fff88) = 0 ({50,133,0,0})
650645 ioctl(1,TIOCGWINSZ,0x407ff750) = 0 ({50,133,0,0})
650645 writev(1,0x407ff720,0x2)                       Ultimate Packer for eXecutables
                          Copyright (C) 1996 - 2023
 = 107
650645 writev(1,0x407ff730,0x2)UPX 4.0.2       Markus Oberhumer, Laszlo Molnar & John Reiser   Jan 30th 2023

 = 79
650645 writev(1,0x407ff730,0x2)        File size         Ratio      Format      Name
 = 54
650645 writev(1,0x407ff730,0x2)   --------------------   ------   -----------   -----------
 = 61
650645 statx(AT_FDCWD,"this-file-does-not-exist",AT_SYMLINK_NOFOLLOW|AT_STATX_SYNC_AS_STAT,STATX_BASIC_STATS,0x407ff2c0) = -1 errno=2 (No such file or directory)
650645 ioctl(2,TIOCGWINSZ,0x407ff310) = 0 ({50,133,0,0})
650645 writev(2,0x407ff2c8,0x2)upx:  = 5
650645 writev(2,0x407ff2c8,0x2)this-file-does-not-exist:  = 26
650645 writev(2,0x407ff2c8,0x2)FileNotFoundException: this-file-does-not-exist: No such file or directory = 74
650645 writev(2,0x407ff2c8,0x2)
 = 1

   << infinite loop in qemu, 100% cpu>>
^C   ## get attention of gdb
Thread 1 "qemu-armeb" received signal SIGINT, Interrupt.
cpu_exec_loop (cpu=cpu@entry=0x55555583bd40, sc=<optimized out>) at ../accel/tcg/cpu-exec.c:990
990	                cflags = curr_cflags(cpu);
Missing separate debuginfos, use: dnf debuginfo-install glib2-2.72.3-1.fc36.x86_64 pcre-8.45-1.fc36.1.x86_64

(gdb) bt
#0  cpu_exec_loop (cpu=cpu@entry=0x55555583bd40, sc=<optimized out>) at ../accel/tcg/cpu-exec.c:990
#1  0x000055555565ef35 in cpu_exec_setjmp (cpu=cpu@entry=0x55555583bd40, sc=<optimized out>) at ../accel/tcg/cpu-exec.c:1057
#2  0x000055555565f428 in cpu_exec (cpu=cpu@entry=0x55555583bd40) at ../accel/tcg/cpu-exec.c:1083
#3  0x000055555559c7f0 in cpu_loop (env=env@entry=0x55555583c070) at ../linux-user/arm/cpu_loop.c:323
#4  0x000055555559880d in main (argc=<optimized out>, argv=0x7fffffffde88, envp=<optimized out>) at ../linux-user/main.c:973

So that is what Markus is complaining about.

@hdeller
Copy link

hdeller commented Jul 20, 2023 via email

@markus-oberhumer
Copy link
Collaborator Author

markus-oberhumer commented Jul 21, 2023

After some more testing it seems that qemu-armeb is just broken since qemu-7, probably because not too many people care about armeb these days.

qemu-6.1.1 (Alpine Linux 3.15) works.

I have no interest further debugging this, closing.

@markus-oberhumer markus-oberhumer closed this as not planned Won't fix, can't repro, duplicate, stale Jul 21, 2023
@markus-oberhumer markus-oberhumer added the notabug The issue is not a bug. label Jul 21, 2023
@markus-oberhumer
Copy link
Collaborator Author

markus-oberhumer commented Jul 21, 2023

Just in case somebody is interested, here is a current upx armeb build (unpacked, with debug symbols). Works fine with qemu-6, won't even start on qemu >= 7

upx-static-armeb.zip

@hdeller
Copy link

hdeller commented Jul 21, 2023 via email

@markus-oberhumer
Copy link
Collaborator Author

Sure, I only closed it because I have no time working on it.

@markus-oberhumer
Copy link
Collaborator Author

No improvements with qemu-8.1.0-rc1, still hangs.

@jreiser
Copy link
Collaborator

jreiser commented Jul 27, 2023

It's a two-instruction loop, with pc==lr (r15==r14), and constant stack pointer R13=407ffad8.

$ QEMU_LOG="in_asm,int,cpu" QEMU_LOG_FILENAME=logfile qemu/build/qemu-armeb -strace upx-4.0.2-armeb_linux/upx this-file-does-not-exist

----------------
IN: 
0x000e55e0:  
OBJD-T: e5954000e1a02005e1a00004e0841006e12fff37

R00=ffffffff R01=00000000 R02=0029af20 R03=000d8dd8
R04=00000001 R05=0029af20 R06=ffffffff R07=ffff0fc0
R08=00000000 R09=002090f0 R10=00000001 R11=0029afa0
R12=0a000000 R13=407ffad8 R14=000e55f4 R15=000e55e0
PSR=a0000010 N-C- A usr32 
R00=00000001 R01=00000000 R02=0029af20 R03=000d8dd8
R04=00000001 R05=0029af20 R06=ffffffff R07=ffff0fc0
R08=00000000 R09=002090f0 R10=00000001 R11=0029afa0
R12=0a000000 R13=407ffad8 R14=000e55f4 R15=ffff0fc0
PSR=a0000010 N-C- A usr32 
R00=ffffffff R01=00000000 R02=0029af20 R03=000d8dd8
R04=00000001 R05=0029af20 R06=ffffffff R07=ffff0fc0
R08=00000000 R09=002090f0 R10=00000001 R11=0029afa0
R12=0a000000 R13=407ffad8 R14=000e55f4 R15=000e55f4
PSR=00000010 ---- A usr32 

# The following two instructions repeat infinitely
R00=00000001 R01=00000000 R02=0029af20 R03=000d8dd8
R04=00000001 R05=0029af20 R06=ffffffff R07=ffff0fc0
R08=00000000 R09=002090f0 R10=00000001 R11=0029afa0
R12=0a000000 R13=407ffad8 R14=000e55f4 R15=ffff0fc0
PSR=a0000010 N-C- A usr32
R00=ffffffff R01=00000000 R02=0029af20 R03=000d8dd8
R04=00000001 R05=0029af20 R06=ffffffff R07=ffff0fc0
R08=00000000 R09=002090f0 R10=00000001 R11=0029afa0
R12=0a000000 R13=407ffad8 R14=000e55f4 R15=000e55f4
PSR=00000010 ---- A usr32 

@hdeller
Copy link

hdeller commented Jul 27, 2023

John, if you

  • build capstone (on your host), see https://github.com/capstone-engine/capstone/tree/v5, and
  • make the static built library available to qemu (e.g. copy it to /usr/lib64/libcapstone.a), then
  • configure qemu with --enable-capstone,
  • and rebuild qemu, then
    qemu will disassemble at runtime the
    IN: 0x000e55e0:
    OBJD-T: e5954000e1a02005e1a00004e0841006e12fff37
    instructions to their mnemonics, which will help to understand what those instructions are.

@hdeller
Copy link

hdeller commented Jul 27, 2023

btw, what is the expected behaviour?
It seems the binaries does a

  • statx(AT_FDCWD,"this-file-does-not exist",AT_SYMLINK_NOFOLLOW|AT_STATX_SYNC_AS_STAT,STATX_BASIC_STATS,0x407ff2c0) = -1 errno=2 (No such file or directory)
  • then writes on the console " this-file-does-not-exist:\n FileNotFoundException: this-file-does-not-exist: No such file or directory = 74"
  • and then seems to hang in a loop.

What is expected?
a) the file-not-found is expected and then it should exit?, or
b) the file-not-found is wrong?
Helge

@markus-oberhumer
Copy link
Collaborator Author

@hdeller It's supposed to continue after the exception has been caught and printed. Just try any qemu-6 or qemu-5 version.

@hdeller
Copy link

hdeller commented Jul 27, 2023

This seems to be a memory-(instruction)-overwrite, either triggered by qemu or from upx.

In qemu-7 I see this instruction:
0xffff0fc0: e92d41f0 push {r4, r5, r6, r7, r8, lr}

and when running qemu-8 (head) I see:
0xffff0fc0: 00000000 andeq r0, r0, r0

after that of course qemu executes other code paths.
So, how/why does 000000 is stored into address 0xffff0fc0 ?

@hdeller
Copy link

hdeller commented Jul 27, 2023

the assembly before that is:
0x000e55d0: e92d41f0 push {r4, r5, r6, r7, r8, lr}
0x000e55d4: e1a05000 mov r5, r0
0x000e55d8: e1a06001 mov r6, r1
0x000e55dc: e59f7020 ldr r7, [pc, #0x20]
0x000e55e0: e5954000 ldr r4, [r5]
0x000e55e4: e1a02005 mov r2, r5
0x000e55e8: e1a00004 mov r0, r4
0x000e55ec: e0841006 add r1, r4, r6
0x000e55f0: e12fff37 blx r7

it seems blx jumps to [r7], in both qemu versions this is the same address 0xffff0fc0, so this is correct.
But why does the content of 0xffff0fc0 changes at runtime?
Based on the address it seems to be some vdso or other library function. Maybe on stack.
I don't know arm assembly, so maybe someone has an idea?

@markus-oberhumer
Copy link
Collaborator Author

markus-oberhumer commented Jul 27, 2023

Please try the attached upx-static-armeb.zip - this is compiled with debug info.

EDIT: this behaves differently because I added an early check for working C++ exceptions, so this now hangs at program startup

EDIT 2: qemu-6.2.0 works with this file; qemu 7.1.0, 7.2.4, 8.0 and 8.1 do not

@hdeller
Copy link

hdeller commented Jul 27, 2023

This is how I run it:
QEMU_LOG="in_asm,int" ./qemu-armeb-8.0 -strace ./upx-static-armeb this-file-does-not-exist 2>&1 | tee L8
output log "L8" is attached.
L8.txt

@markus-oberhumer
Copy link
Collaborator Author

Update: I have attached new test files (4 builds), including gcc-11 builds

upx-devel-build-armeb-static.zip

qemu-6 still works on all of these 4 files

but qemu-8.1 now crashes, maybe this can help debugging:

$ /usr/local/packages/qemu-user-8.1.0-rc1/bin/qemu-armeb ./upx-devel-build-armeb-static/linux-musl-gcc-11/armeb-linux-musleabi-gcc-11.3.0/debug/upx
qemu: uncaught target signal 4 (Illegal instruction) - core dumped
zsh: illegal hardware instruction (core dumped)  /usr/local/packages/qemu-user-8.1.0-rc1/bin/qemu-armeb

P.S. .tar.xz within .zip file beause of GitHub limitation

@markus-oberhumer
Copy link
Collaborator Author

Another update: qemu-7.0.0 also does NOT work, so the problem (regression?) was introduced between 6.2.0 and 7.0.0

@hdeller
Copy link

hdeller commented Jul 27, 2023

Could you run a "git bisect" on qemu between 6.0 and 7.0 ?
That way we find the bad commit...

@jreiser
Copy link
Collaborator

jreiser commented Jul 27, 2023

Running "git bisect" is not as simple as implied by the documentation "git help bisect". You have to use the compiler and other tools that match the version that you're trying to build. For example, qemu v6.2.0 does not build with gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4). Supposedly v6.2.0 is good, but that should be verified by actual re-build.

$ git checkout v6.2.0
$ ./configure
$ make -j4 qemu-armeb
[496/747] Compiling C object libqemuutil.a.p/qobject_block-qdict.c.o
FAILED: libqemuutil.a.p/qobject_block-qdict.c.o 
cc -m64 -mcx16 -Ilibqemuutil.a.p -I. -I.. -Isubprojects/libvhost-user -I../subprojects/libvhost-user -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/p11-kit-1 -I/usr/include/pixman-1 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /bigdata/qemu/linux-headers -isystem linux-headers -iquote . -iquote /bigdata/qemu -iquote /bigdata/qemu/include -iquote /bigdata/qemu/disas/libvixl -iquote /bigdata/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -MD -MQ libqemuutil.a.p/qobject_block-qdict.c.o -MF libqemuutil.a.p/qobject_block-qdict.c.o.d -o libqemuutil.a.p/qobject_block-qdict.c.o -c ../qobject/block-qdict.c
In file included from /bigdata/qemu/include/qapi/qmp/qdict.h:16,
                 from /bigdata/qemu/include/block/qdict.h:13,
                 from ../qobject/block-qdict.c:11:
/bigdata/qemu/include/qapi/qmp/qobject.h: In function ‘qdict_array_split’:
/bigdata/qemu/include/qapi/qmp/qobject.h:49:17: error: ‘subqdict’ may be used uninitialized [-Werror=maybe-uninitialized]
   49 |     typeof(obj) _obj = (obj);                                   \
      |                 ^~~~
../qobject/block-qdict.c:227:16: note: ‘subqdict’ declared here
  227 |         QDict *subqdict;
      |                ^~~~~~~~
cc1: all warnings being treated as errors
[497/747] Compiling C object libqemuutil.a.p/util_osdep.c.o
[498/747] Compiling C object libqemuutil.a.p/qobject_json-parser.c.o
[499/747] Compiling C object libqemuutil.a.p/util_cutils.c.o
ninja: build stopped: subcommand failed.
make[1]: *** [Makefile:162: run-ninja] Error 1
make[1]: Leaving directory '/bigdata/qemu/build'
make: *** [GNUmakefile:11: qemu-armeb] Error 2

@hdeller
Copy link

hdeller commented Jul 27, 2023

yes, I know.
This specific issue I worked around by initializing adding " *sibqdict = NULL;" in qobject.h:
/bigdata/qemu/include/qapi/qmp/qobject.h:49:17: error: ‘subqdict’

I needed also "--disable-werror" (or something like that).

@hdeller
Copy link

hdeller commented Jul 27, 2023

But I think you need to compare 6.0 (not 6.2) to 7.0, since 6.2 is another branch.

@markus-oberhumer
Copy link
Collaborator Author

git merge-base v6.2.0 v7.0.0 points to 44f28df24767cf9dca1ddc9b23157737c4cbb645 which is v6.2.0.

@markus-oberhumer
Copy link
Collaborator Author

markus-oberhumer commented Jul 27, 2023

git log --format=oneline v6.2.0..v7.0.0 says there are 2751 commits, so bisect should succeed in 12 steps.

@jreiser
Copy link
Collaborator

jreiser commented Jul 27, 2023

working... 5 steps remaining.

@jreiser
Copy link
Collaborator

jreiser commented Jul 27, 2023

$ git bisect bad
7f4f0d9ea87097a31b36bcc52b7368efed35593c is the first bad commit
commit 7f4f0d9ea87097a31b36bcc52b7368efed35593c
Author: Richard Henderson <richard.henderson@linaro.org>
Date:   Tue Mar 22 17:58:38 2022 -0700

    linux-user/arm: Implement __kernel_cmpxchg with host atomics
    
    The existing implementation using start/end_exclusive
    does not provide atomicity across processes.
    
    Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
    Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
    Message-Id: <20220323005839.94327-3-richard.henderson@linaro.org>
    Signed-off-by: Laurent Vivier <laurent@vivier.eu>

 linux-user/arm/cpu_loop.c | 87 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 62 insertions(+), 25 deletions(-)


# The bisect steps were:
-rw-r--r--. 1 jreiser jreiser   435478 Jul 27 11:57 7f4f0d9ea8.log
-rw-r--r--. 1 jreiser jreiser   435478 Jul 27 11:50 330ea9d1d8.log
-rw-r--r--. 1 jreiser jreiser   435476 Jul 27 11:47 9c72129150.log
-rw-r--r--. 1 jreiser jreiser 10352915 Jul 27 11:44 e309ce90a2.log
-rw-r--r--. 1 jreiser jreiser 10352901 Jul 27 11:32 6e05e7047c.log
-rw-r--r--. 1 jreiser jreiser 10352915 Jul 27 11:29 efd080de76.log
-rw-r--r--. 1 jreiser jreiser   435478 Jul 27 11:27 10c473246b.log
-rw-r--r--. 1 jreiser jreiser   435464 Jul 27 11:24 97cd74f772.log
-rw-r--r--. 1 jreiser jreiser 10352915 Jul 27 11:19 17e6ffa6a5.log
-rw-r--r--. 1 jreiser jreiser 10352915 Jul 27 11:17 9740b907a5.log
-rw-r--r--. 1 jreiser jreiser 10352915 Jul 27 11:13 477c3b934a.log
-rw-r--r--. 1 jreiser jreiser 10352905 Jul 27 11:10 d70075373a.log
-rw-r--r--. 1 jreiser jreiser 10352915 Jul 27 11:06 0021c4765a.log
-rw-r--r--. 1 jreiser jreiser 10352811 Jul 27 11:01 v6.0.0.log

Yes, that *.log info is redundant, IF there were no mistakes in the ~50 step process (12 git versions, 4 steps per version.)
I'll save the .log for a while. (Short .log were bad, long .log were good.)

@markus-oberhumer
Copy link
Collaborator Author

@jreiser Great job!

@hdeller
Copy link

hdeller commented Jul 27, 2023

I'm sure some endianess fixup is missing in this commit for armeb...

@markus-oberhumer
Copy link
Collaborator Author

CC @rth7680 Richard Henderson, asking for feedback

@hdeller
Copy link

hdeller commented Jul 27, 2023

try this patch:

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index a992423257..ff0bff7c63 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -117,8 +117,8 @@ static void arm_kernel_cmpxchg32_helper(CPUARMState *env)
 {
     uint32_t oldval, newval, val, addr, cpsr, *host_addr;
 
-    oldval = env->regs[0];
-    newval = env->regs[1];
+    oldval = tswap32(env->regs[0]);
+    newval = tswap32(env->regs[1]);
     addr = env->regs[2];

@hdeller
Copy link

hdeller commented Jul 27, 2023

Yes, patch above works for me as expected with qemu v8-rc:
./qemu-armeb ./upx-static-armeb this-file-does-not-exist

[deller@p100 qemu-helge-user-armeb]$ ./qemu-armeb ./upx-static-armeb  this-file-does-not-exist
                       Ultimate Packer for eXecutables
                          Copyright (C) 1996 - 2023
UPX git-f123fd  Markus Oberhumer, Laszlo Molnar & John Reiser   May 28th 2023

        File size         Ratio      Format      Name
   --------------------   ------   -----------   -----------
upx-static-armeb: this-file-does-not-exist: FileNotFoundException: this-file-does-not-exist: No such file or directory

Packed 0 files.

WARNING: this is an unstable beta version - use for testing only! Really.

@markus-oberhumer
Copy link
Collaborator Author

Please also try the 4 updated test files I posted above:

upx-devel-build-armeb-static.zip

@hdeller
Copy link

hdeller commented Jul 27, 2023

all of those work too, show some doctest stiuff with SUCCESS, and then finish with failure without hang

@markus-oberhumer
Copy link
Collaborator Author

markus-oberhumer commented Jul 27, 2023

Great!

(doctest is a C++ testing framework, and UPX Debug builds default to showing the [doctest] summary)

@jreiser
Copy link
Collaborator

jreiser commented Jul 27, 2023

Against v8.1.0-rc1:

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index a992423257..06e5330de2 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -117,9 +117,9 @@ static void arm_kernel_cmpxchg32_helper(CPUARMState *env)
 {
     uint32_t oldval, newval, val, addr, cpsr, *host_addr;
 
-    oldval = env->regs[0];
-    newval = env->regs[1];
-    addr = env->regs[2];
+    oldval = tswap32(env->regs[0]);
+    newval = tswap32(env->regs[1]);
+    addr = tswap32(env->regs[2]);
 
     mmap_lock();
     host_addr = atomic_mmu_lookup(env, addr, 4);
@@ -132,8 +132,8 @@ static void arm_kernel_cmpxchg32_helper(CPUARMState *env)
     mmap_unlock();
 
     cpsr = (val == oldval) * CPSR_C;
-    cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
-    env->regs[0] = cpsr ? 0 : -1;
+    cpsr_write(env, tswap32(cpsr), CPSR_C, CPSRWriteByInstr);
+    env->regs[0] = cpsr ? 0 : -1;  /* all bits the same; no swap neede */
 }
 
 /*

It also seems to me that ./target/arm/helper.c:void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
has DOZENS more places where tswap32 is required: basically ANY reference to *env. (Remove my call to tswap32 inside the call to cpsr_write, once cpsr_write solves the general problem.)

@hdeller
Copy link

hdeller commented Jul 27, 2023

No, your patch is wrong. You should not swap the "addr" variable. It's already the virtual address which is the same in the emulator. I think cpsr shouldn't be swapped either, but Richard should know better.

@jreiser
Copy link
Collaborator

jreiser commented Jul 27, 2023

I defer to maintainers; obviously they should know. And just as obviously, they should COMMENT THE CODE about why some references to *env get tswap32, and others do not.

@markus-oberhumer
Copy link
Collaborator Author

markus-oberhumer commented Jul 27, 2023

@jreiser I'm sure you are aware that endian issues are a nasty problem. The linux kernel started using __be32/__le32 quite some time ago (but I think this needs some extra checker, not sure). In UPX we greatly benefit from using C++ because we can use those nice BE32/LE32 classes.

@hdeller
Copy link

hdeller commented Jul 27, 2023

oldval and newval needs to be swapped, because they are written & read with host-endianess (via the qatomic_cmpxchg__nocheck() function) into the guest memory and the guest expects (for armeb) words stored in big-endian.

@markus-oberhumer
Copy link
Collaborator Author

Closing, thanks to everyone!

@markus-oberhumer
Copy link
Collaborator Author

markus-oberhumer commented Jul 29, 2023

stsquad pushed a commit to qemu/qemu that referenced this issue Jul 31, 2023
Commit 7f4f0d9 ("linux-user/arm: Implement __kernel_cmpxchg with host
atomics") switched to use qatomic_cmpxchg() to swap a word with the memory
content, but missed to endianess-swap the oldval and newval values when
emulating an armeb CPU, which expects words to be stored in big endian in
the guest memory.

The bug can be verified with qemu >= v7.0 on any little-endian host, when
starting the armeb binary of the upx program, which just hangs without
this patch.

Cc: qemu-stable@nongnu.org
Signed-off-by: Helge Deller <deller@gmx.de>
Reported-by: "Markus F.X.J. Oberhumer" <markus@oberhumer.com>
Reported-by: John Reiser <jreiser@BitWagon.com>
Closes: upx/upx#687
Message-Id: <ZMQVnqY+F+5sTNFd@p100>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
stsquad pushed a commit to qemu/qemu that referenced this issue Aug 1, 2023
Commit 7f4f0d9 ("linux-user/arm: Implement __kernel_cmpxchg with host
atomics") switched to use qatomic_cmpxchg() to swap a word with the memory
content, but missed to endianess-swap the oldval and newval values when
emulating an armeb CPU, which expects words to be stored in big endian in
the guest memory.

The bug can be verified with qemu >= v7.0 on any little-endian host, when
starting the armeb binary of the upx program, which just hangs without
this patch.

Cc: qemu-stable@nongnu.org
Signed-off-by: Helge Deller <deller@gmx.de>
Reported-by: "Markus F.X.J. Oberhumer" <markus@oberhumer.com>
Reported-by: John Reiser <jreiser@BitWagon.com>
Closes: upx/upx#687
Message-Id: <ZMQVnqY+F+5sTNFd@p100>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
(cherry picked from commit 38dd78c)
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
@markus-oberhumer
Copy link
Collaborator Author

@hdeller Please make sure the patch also lands in the qemu origin/staging-8.0 branch, it currently is only commited in origin/staging-7.2 and origin/staging.

@hdeller
Copy link

hdeller commented Aug 3, 2023 via email

@markus-oberhumer
Copy link
Collaborator Author

I'm talking about armeb and this commit
https://gitlab.com/qemu-project/qemu/-/commit/38dd78c41eaf08b490c9e7ec68fc508bbaa5cb1d
which I cannot find in staging-8.0 or Debian.

~Markus

@hdeller
Copy link

hdeller commented Aug 3, 2023 via email

stsquad pushed a commit to qemu/qemu that referenced this issue Aug 4, 2023
Commit 7f4f0d9 ("linux-user/arm: Implement __kernel_cmpxchg with host
atomics") switched to use qatomic_cmpxchg() to swap a word with the memory
content, but missed to endianess-swap the oldval and newval values when
emulating an armeb CPU, which expects words to be stored in big endian in
the guest memory.

The bug can be verified with qemu >= v7.0 on any little-endian host, when
starting the armeb binary of the upx program, which just hangs without
this patch.

Cc: qemu-stable@nongnu.org
Signed-off-by: Helge Deller <deller@gmx.de>
Reported-by: "Markus F.X.J. Oberhumer" <markus@oberhumer.com>
Reported-by: John Reiser <jreiser@BitWagon.com>
Closes: upx/upx#687
Message-Id: <ZMQVnqY+F+5sTNFd@p100>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
(cherry picked from commit 38dd78c)
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
stsquad pushed a commit to qemu/qemu that referenced this issue Aug 4, 2023
Commit 7f4f0d9 ("linux-user/arm: Implement __kernel_cmpxchg with host
atomics") switched to use qatomic_cmpxchg() to swap a word with the memory
content, but missed to endianess-swap the oldval and newval values when
emulating an armeb CPU, which expects words to be stored in big endian in
the guest memory.

The bug can be verified with qemu >= v7.0 on any little-endian host, when
starting the armeb binary of the upx program, which just hangs without
this patch.

Cc: qemu-stable@nongnu.org
Signed-off-by: Helge Deller <deller@gmx.de>
Reported-by: "Markus F.X.J. Oberhumer" <markus@oberhumer.com>
Reported-by: John Reiser <jreiser@BitWagon.com>
Closes: upx/upx#687
Message-Id: <ZMQVnqY+F+5sTNFd@p100>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
(cherry picked from commit 38dd78c)
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notabug The issue is not a bug. os:Linux
Projects
None yet
Development

No branches or pull requests

3 participants