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

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

[Ilya Leoshkevich]

v2: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg07017.html
v2 -> v3: Fix __put_user() argument order mixup.
          I tested v2 only against sh4, where the original problem was
          discovered, but it regresses aarch64.

v1: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg06999.html
v1 -> v2: Fix by using proper target_rlimit64 alignment (Richard).
          Use __get_user() and __put_user() (Philippe - if I understood
          the suggestion correctly).

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/generic/target_resource.h        |  4 +-
 linux-user/syscall.c                        |  8 ++--
 tests/tcg/multiarch/linux/linux-fork-trap.c | 51 +++++++++++++++++++++
 3 files changed, 57 insertions(+), 6 deletions(-)
 create mode 100644 tests/tcg/multiarch/linux/linux-fork-trap.c

-- 
2.39.1

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

[Ilya Leoshkevich]

target_rlimit64 contains uint64_t fields, so it's 8-byte aligned on
some hosts, while some guests may align their respective type on a
4-byte boundary. This may lead to an unaligned access, which is an UB.

Fix by defining the fields as abi_ullong. This makes the host alignment
match that of the guest, and lets the compiler know that it should emit
code that can deal with the guest alignment.

While at it, also use __get_user() and __put_user() instead of
tswap64().

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/generic/target_resource.h | 4 ++--
 linux-user/syscall.c                 | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/linux-user/generic/target_resource.h b/linux-user/generic/target_resource.h
index 539d8c46772..37d3eb09b3b 100644
--- a/linux-user/generic/target_resource.h
+++ b/linux-user/generic/target_resource.h
@@ -12,8 +12,8 @@ struct target_rlimit {
 };
 
 struct target_rlimit64 {
-    uint64_t rlim_cur;
-    uint64_t rlim_max;
+    abi_ullong rlim_cur;
+    abi_ullong rlim_max;
 };
 
 #define TARGET_RLIM_INFINITY    ((abi_ulong)-1)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a6c426d73cf..73082531ffc 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -12886,8 +12886,8 @@ 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);
+            __get_user(rnew.rlim_cur, &target_rnew->rlim_cur);
+            __get_user(rnew.rlim_max, &target_rnew->rlim_max);
             unlock_user_struct(target_rnew, arg3, 0);
             rnewp = &rnew;
         }
@@ -12897,8 +12897,8 @@ 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);
+            __put_user(rold.rlim_cur, &target_rold->rlim_cur);
+            __put_user(rold.rlim_max, &target_rold->rlim_max);
             unlock_user_struct(target_rold, arg4, 1);
         }
         return ret;
-- 
2.39.1

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

[Philippe Mathieu-Daudé]

On 24/2/23 01:39, Ilya Leoshkevich wrote:
> target_rlimit64 contains uint64_t fields, so it's 8-byte aligned on
> some hosts, while some guests may align their respective type on a
> 4-byte boundary. This may lead to an unaligned access, which is an UB.
> 
> Fix by defining the fields as abi_ullong. This makes the host alignment
> match that of the guest, and lets the compiler know that it should emit
> code that can deal with the guest alignment.
> 
> While at it, also use __get_user() and __put_user() instead of
> tswap64().
> 
> 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/generic/target_resource.h | 4 ++--
>   linux-user/syscall.c                 | 8 ++++----
>   2 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Laurent Vivier]

Le 24/02/2023 à 01:39, Ilya Leoshkevich a écrit :
> target_rlimit64 contains uint64_t fields, so it's 8-byte aligned on
> some hosts, while some guests may align their respective type on a
> 4-byte boundary. This may lead to an unaligned access, which is an UB.
> 
> Fix by defining the fields as abi_ullong. This makes the host alignment
> match that of the guest, and lets the compiler know that it should emit
> code that can deal with the guest alignment.
> 
> While at it, also use __get_user() and __put_user() instead of
> tswap64().
> 
> 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/generic/target_resource.h | 4 ++--
>   linux-user/syscall.c                 | 8 ++++----
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/generic/target_resource.h b/linux-user/generic/target_resource.h
> index 539d8c46772..37d3eb09b3b 100644
> --- a/linux-user/generic/target_resource.h
> +++ b/linux-user/generic/target_resource.h
> @@ -12,8 +12,8 @@ struct target_rlimit {
>   };
>   
>   struct target_rlimit64 {
> -    uint64_t rlim_cur;
> -    uint64_t rlim_max;
> +    abi_ullong rlim_cur;
> +    abi_ullong rlim_max;
>   };
>   
>   #define TARGET_RLIM_INFINITY    ((abi_ulong)-1)
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a6c426d73cf..73082531ffc 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -12886,8 +12886,8 @@ 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);
> +            __get_user(rnew.rlim_cur, &target_rnew->rlim_cur);
> +            __get_user(rnew.rlim_max, &target_rnew->rlim_max);
>               unlock_user_struct(target_rnew, arg3, 0);
>               rnewp = &rnew;
>           }
> @@ -12897,8 +12897,8 @@ 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);
> +            __put_user(rold.rlim_cur, &target_rold->rlim_cur);
> +            __put_user(rold.rlim_max, &target_rold->rlim_max);
>               unlock_user_struct(target_rold, arg4, 1);
>           }
>           return ret;

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

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

[Laurent Vivier]

Le 24/02/2023 à 01:39, Ilya Leoshkevich a écrit :
> target_rlimit64 contains uint64_t fields, so it's 8-byte aligned on
> some hosts, while some guests may align their respective type on a
> 4-byte boundary. This may lead to an unaligned access, which is an UB.
> 
> Fix by defining the fields as abi_ullong. This makes the host alignment
> match that of the guest, and lets the compiler know that it should emit
> code that can deal with the guest alignment.
> 
> While at it, also use __get_user() and __put_user() instead of
> tswap64().
> 
> 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/generic/target_resource.h | 4 ++--
>   linux-user/syscall.c                 | 8 ++++----
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/generic/target_resource.h b/linux-user/generic/target_resource.h
> index 539d8c46772..37d3eb09b3b 100644
> --- a/linux-user/generic/target_resource.h
> +++ b/linux-user/generic/target_resource.h
> @@ -12,8 +12,8 @@ struct target_rlimit {
>   };
>   
>   struct target_rlimit64 {
> -    uint64_t rlim_cur;
> -    uint64_t rlim_max;
> +    abi_ullong rlim_cur;
> +    abi_ullong rlim_max;
>   };
>   
>   #define TARGET_RLIM_INFINITY    ((abi_ulong)-1)
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a6c426d73cf..73082531ffc 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -12886,8 +12886,8 @@ 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);
> +            __get_user(rnew.rlim_cur, &target_rnew->rlim_cur);
> +            __get_user(rnew.rlim_max, &target_rnew->rlim_max);
>               unlock_user_struct(target_rnew, arg3, 0);
>               rnewp = &rnew;
>           }
> @@ -12897,8 +12897,8 @@ 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);
> +            __put_user(rold.rlim_cur, &target_rold->rlim_cur);
> +            __put_user(rold.rlim_max, &target_rold->rlim_max);
>               unlock_user_struct(target_rold, arg4, 1);
>           }
>           return ret;

Applied to my linux-user-for-8.0 branch.

Thanks,
Laurent

[PATCH v3 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