[PATCH 0/2] linux-user: Fix unaligned memory access in prlimit64 syscall

[PATCH 0/2] linux-user: Fix unaligned memory access in prlimit64 syscall

[Ilya Leoshkevich]

Hi,

Richard reported [1] that the new linux-fork-trap test was failing
under UBSan [2], so it was excluded from the PR.

This is a resend of the test plus the fix for the additional issue that
it uncovered.

[1] https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg06130.html
[2] https://gitlab.com/qemu-project/qemu/-/jobs/3807471447#L5064

Best regards,
Ilya

Ilya Leoshkevich (2):
  linux-user: Fix unaligned memory access in prlimit64 syscall
  tests/tcg/linux-test: Add linux-fork-trap test

 linux-user/syscall.c                        | 12 +++--
 tests/tcg/multiarch/linux/linux-fork-trap.c | 51 +++++++++++++++++++++
 2 files changed, 58 insertions(+), 5 deletions(-)
 create mode 100644 tests/tcg/multiarch/linux/linux-fork-trap.c

-- 
2.39.1

[PATCH 1/2] linux-user: Fix unaligned memory access in prlimit64 syscall

[Ilya Leoshkevich]

32-bit guests may enforce only 4-byte alignment for target_rlimit64,
whereas 64-bit hosts normally require the 8-byte one. Therefore
accessing this struct directly is UB.

Fix by adding a local copy.

Fixes: 163a05a8398b ("linux-user: Implement prlimit64 syscall")
Reported-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/syscall.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a6c426d73cf..8ae7696d8f1 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -12876,7 +12876,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
     case TARGET_NR_prlimit64:
     {
         /* args: pid, resource number, ptr to new rlimit, ptr to old rlimit */
-        struct target_rlimit64 *target_rnew, *target_rold;
+        struct target_rlimit64 *target_rnew, *target_rold, tmp;
         struct host_rlimit64 rnew, rold, *rnewp = 0;
         int resource = target_to_host_resource(arg2);
 
@@ -12886,8 +12886,9 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
             if (!lock_user_struct(VERIFY_READ, target_rnew, arg3, 1)) {
                 return -TARGET_EFAULT;
             }
-            rnew.rlim_cur = tswap64(target_rnew->rlim_cur);
-            rnew.rlim_max = tswap64(target_rnew->rlim_max);
+            memcpy(&tmp, target_rnew, sizeof(tmp));
+            rnew.rlim_cur = tswap64(tmp.rlim_cur);
+            rnew.rlim_max = tswap64(tmp.rlim_max);
             unlock_user_struct(target_rnew, arg3, 0);
             rnewp = &rnew;
         }
@@ -12897,8 +12898,9 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
             if (!lock_user_struct(VERIFY_WRITE, target_rold, arg4, 1)) {
                 return -TARGET_EFAULT;
             }
-            target_rold->rlim_cur = tswap64(rold.rlim_cur);
-            target_rold->rlim_max = tswap64(rold.rlim_max);
+            tmp.rlim_cur = tswap64(rold.rlim_cur);
+            tmp.rlim_max = tswap64(rold.rlim_max);
+            memcpy(target_rold, &tmp, sizeof(*target_rold));
             unlock_user_struct(target_rold, arg4, 1);
         }
         return ret;
-- 
2.39.1

[PATCH 2/2] tests/tcg/linux-test: Add linux-fork-trap test

[Ilya Leoshkevich]

Check that dying due to a signal does not deadlock.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/multiarch/linux/linux-fork-trap.c | 51 +++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 tests/tcg/multiarch/linux/linux-fork-trap.c

diff --git a/tests/tcg/multiarch/linux/linux-fork-trap.c b/tests/tcg/multiarch/linux/linux-fork-trap.c
new file mode 100644
index 00000000000..2bfef800c3e
--- /dev/null
+++ b/tests/tcg/multiarch/linux/linux-fork-trap.c
@@ -0,0 +1,51 @@
+/*
+ * Test that a fork()ed process terminates after __builtin_trap().
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/resource.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+int main(void)
+{
+    struct rlimit nodump;
+    pid_t err, pid;
+    int wstatus;
+
+    pid = fork();
+    assert(pid != -1);
+    if (pid == 0) {
+        /* We are about to crash on purpose; disable core dumps. */
+        if (getrlimit(RLIMIT_CORE, &nodump)) {
+            return EXIT_FAILURE;
+        }
+        nodump.rlim_cur = 0;
+        if (setrlimit(RLIMIT_CORE, &nodump)) {
+            return EXIT_FAILURE;
+        }
+        /*
+         * An alternative would be to dereference a NULL pointer, but that
+         * would be an UB in C.
+         */
+        printf("about to trigger fault...\n");
+#if defined(__MICROBLAZE__)
+        /*
+         * gcc emits "bri 0", which is an endless loop.
+         * Take glibc's ABORT_INSTRUCTION.
+         */
+        asm volatile("brki r0,-1");
+#else
+        __builtin_trap();
+#endif
+    }
+    err = waitpid(pid, &wstatus, 0);
+    assert(err == pid);
+    assert(WIFSIGNALED(wstatus));
+    printf("faulting thread exited cleanly\n");
+
+    return EXIT_SUCCESS;
+}
-- 
2.39.1

Re: [PATCH 1/2] linux-user: Fix unaligned memory access in prlimit64 syscall

[Philippe Mathieu-Daudé]

On 23/2/23 22:58, Ilya Leoshkevich wrote:
> 32-bit guests may enforce only 4-byte alignment for target_rlimit64,
> whereas 64-bit hosts normally require the 8-byte one. Therefore
> accessing this struct directly is UB.
> 
> Fix by adding a local copy.

Shouldn't we fix that globally in __get_user/__put_user?

> Fixes: 163a05a8398b ("linux-user: Implement prlimit64 syscall")
> Reported-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/syscall.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a6c426d73cf..8ae7696d8f1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -12876,7 +12876,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>       case TARGET_NR_prlimit64:
>       {
>           /* args: pid, resource number, ptr to new rlimit, ptr to old rlimit */
> -        struct target_rlimit64 *target_rnew, *target_rold;
> +        struct target_rlimit64 *target_rnew, *target_rold, tmp;
>           struct host_rlimit64 rnew, rold, *rnewp = 0;
>           int resource = target_to_host_resource(arg2);
>   
> @@ -12886,8 +12886,9 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>               if (!lock_user_struct(VERIFY_READ, target_rnew, arg3, 1)) {
>                   return -TARGET_EFAULT;
>               }
> -            rnew.rlim_cur = tswap64(target_rnew->rlim_cur);
> -            rnew.rlim_max = tswap64(target_rnew->rlim_max);
> +            memcpy(&tmp, target_rnew, sizeof(tmp));
> +            rnew.rlim_cur = tswap64(tmp.rlim_cur);
> +            rnew.rlim_max = tswap64(tmp.rlim_max);
>               unlock_user_struct(target_rnew, arg3, 0);
>               rnewp = &rnew;
>           }
> @@ -12897,8 +12898,9 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>               if (!lock_user_struct(VERIFY_WRITE, target_rold, arg4, 1)) {
>                   return -TARGET_EFAULT;
>               }
> -            target_rold->rlim_cur = tswap64(rold.rlim_cur);
> -            target_rold->rlim_max = tswap64(rold.rlim_max);
> +            tmp.rlim_cur = tswap64(rold.rlim_cur);
> +            tmp.rlim_max = tswap64(rold.rlim_max);
> +            memcpy(target_rold, &tmp, sizeof(*target_rold));
>               unlock_user_struct(target_rold, arg4, 1);
>           }
>           return ret;

Re: [PATCH 1/2] linux-user: Fix unaligned memory access in prlimit64 syscall

[Ilya Leoshkevich]

On Thu, 2023-02-23 at 23:17 +0100, Philippe Mathieu-Daudé wrote:
> On 23/2/23 22:58, Ilya Leoshkevich wrote:
> > 32-bit guests may enforce only 4-byte alignment for
> > target_rlimit64,
> > whereas 64-bit hosts normally require the 8-byte one. Therefore
> > accessing this struct directly is UB.
> > 
> > Fix by adding a local copy.
> 
> Shouldn't we fix that globally in __get_user/__put_user?

Do you mean replace tswapNN() usages with these functions
in syscall.c? This makes sense.

> 
> > Fixes: 163a05a8398b ("linux-user: Implement prlimit64 syscall")
> > Reported-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   linux-user/syscall.c | 12 +++++++-----
> >   1 file changed, 7 insertions(+), 5 deletions(-)

Re: [PATCH 1/2] linux-user: Fix unaligned memory access in prlimit64 syscall

[Richard Henderson]

On 2/23/23 11:58, Ilya Leoshkevich wrote:
> 32-bit guests may enforce only 4-byte alignment for target_rlimit64,
> whereas 64-bit hosts normally require the 8-byte one. Therefore
> accessing this struct directly is UB.
> 
> Fix by adding a local copy.
> 
> Fixes: 163a05a8398b ("linux-user: Implement prlimit64 syscall")
> Reported-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/syscall.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a6c426d73cf..8ae7696d8f1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -12876,7 +12876,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>       case TARGET_NR_prlimit64:
>       {
>           /* args: pid, resource number, ptr to new rlimit, ptr to old rlimit */
> -        struct target_rlimit64 *target_rnew, *target_rold;
> +        struct target_rlimit64 *target_rnew, *target_rold, tmp;

The bug is that target_rlimit64 uses uint64_t (64-bit host alignment), when it should be 
using abi_ullong (64-bit target alignment).  There are quite a number of these sorts of 
bugs in linux-user.


r~

Re: [PATCH 1/2] linux-user: Fix unaligned memory access in prlimit64 syscall

[Ilya Leoshkevich]

On Thu, 2023-02-23 at 12:31 -1000, Richard Henderson wrote:
> On 2/23/23 11:58, Ilya Leoshkevich wrote:
> > 32-bit guests may enforce only 4-byte alignment for
> > target_rlimit64,
> > whereas 64-bit hosts normally require the 8-byte one. Therefore
> > accessing this struct directly is UB.
> > 
> > Fix by adding a local copy.
> > 
> > Fixes: 163a05a8398b ("linux-user: Implement prlimit64 syscall")
> > Reported-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   linux-user/syscall.c | 12 +++++++-----
> >   1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index a6c426d73cf..8ae7696d8f1 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -12876,7 +12876,7 @@ static abi_long do_syscall1(CPUArchState
> > *cpu_env, int num, abi_long arg1,
> >       case TARGET_NR_prlimit64:
> >       {
> >           /* args: pid, resource number, ptr to new rlimit, ptr to
> > old rlimit */
> > -        struct target_rlimit64 *target_rnew, *target_rold;
> > +        struct target_rlimit64 *target_rnew, *target_rold, tmp;
> 
> The bug is that target_rlimit64 uses uint64_t (64-bit host
> alignment), when it should be 
> using abi_ullong (64-bit target alignment).  There are quite a number
> of these sorts of 
> bugs in linux-user.
> 
> 
> r~

Thanks, this helps.

I thought that unaligned accesses were illegal no matter what, e.g., on
sparc64, but turns out the compiler is actually smart enough to handle
them:

#include <stdint.h>
typedef uint64_t abi_ullong __attribute__((aligned(4)));
abi_ullong load(abi_ullong *x) { return *x; }

produces

load:
	save	%sp, -176, %sp
	lduw	[%i0], %g1
	lduw	[%i0+4], %i0
	sllx	%g1, 32, %g1
	return	%i7+8
	 or	%o0, %g1, %o0

instead of just

load:
	save	%sp, -176, %sp
	return	%i7+8
	 ldx	[%o0], %o0

I'll send a v2.

Best regards,
Ilya