[Qemu-devel] [PATCH for-2.12] linux-user/signal.c: Ensure AArch64 signal frame isn't too small

[Qemu-devel] [PATCH for-2.12] linux-user/signal.c: Ensure AArch64 signal frame isn't too small

[Peter Maydell]

The AArch64 signal frame design was extended for SVE in commit
8c5931de0ac77388096d79ceb, so that instead of having a fixed setup we
now add various records to the frame, with some of them possibly
overflowing into an extra space outside the original 4K reserved
block in the target_sigcontext.  However, we failed to ensure that we
always at least allocate the 4K reserved block.  This is ABI, and
some userspace programs rely on it.  In particular the dash shell
would segfault if the frame wasn't as big enough.

(Compare the kernel's sigframe_size() function in
arch/arm64/kernel/signal.c.)

Reported-by: Richard Henwood <richard.henwood@arm.com>
Fixes: https://bugs.launchpad.net/bugs/1761535
Fixes: 8c5931de0ac77388096d79ceb
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 046d4c8aa0..8d9e6e8410 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1850,6 +1850,12 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
     fr_ofs = layout.total_size;
     layout.total_size += sizeof(struct target_rt_frame_record);
 
+    /* We must always provide at least the standard 4K reserved space,
+     * even if we don't use all of it (this is part of the ABI)
+     */
+    layout.total_size = MAX(layout.total_size,
+                            sizeof(struct target_rt_sigframe));
+
     frame_addr = get_sigframe(ka, env, layout.total_size);
     trace_user_setup_frame(env, frame_addr);
     if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
-- 
2.16.2

Re: [Qemu-devel] [PATCH for-2.12] linux-user/signal.c: Ensure AArch64 signal frame isn't too small

[Laurent Vivier]

Le 09/04/2018 à 16:07, Peter Maydell a écrit :
> The AArch64 signal frame design was extended for SVE in commit
> 8c5931de0ac77388096d79ceb, so that instead of having a fixed setup we
> now add various records to the frame, with some of them possibly
> overflowing into an extra space outside the original 4K reserved
> block in the target_sigcontext.  However, we failed to ensure that we
> always at least allocate the 4K reserved block.  This is ABI, and
> some userspace programs rely on it.  In particular the dash shell
> would segfault if the frame wasn't as big enough.
> 
> (Compare the kernel's sigframe_size() function in
> arch/arm64/kernel/signal.c.)
> 
> Reported-by: Richard Henwood <richard.henwood@arm.com>
> Fixes: https://bugs.launchpad.net/bugs/1761535
> Fixes: 8c5931de0ac77388096d79ceb
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/signal.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 046d4c8aa0..8d9e6e8410 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1850,6 +1850,12 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
>      fr_ofs = layout.total_size;
>      layout.total_size += sizeof(struct target_rt_frame_record);
>  
> +    /* We must always provide at least the standard 4K reserved space,
> +     * even if we don't use all of it (this is part of the ABI)
> +     */
> +    layout.total_size = MAX(layout.total_size,
> +                            sizeof(struct target_rt_sigframe));
> +
>      frame_addr = get_sigframe(ka, env, layout.total_size);
>      trace_user_setup_frame(env, frame_addr);
>      if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
> 

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

Re: [Qemu-devel] [PATCH for-2.12] linux-user/signal.c: Ensure AArch64 signal frame isn't too small

[Richard Henderson]

On 04/10/2018 12:07 AM, Peter Maydell wrote:
> In particular the dash shell
> would segfault if the frame wasn't as big enough.

Ah, that was the critical difference in my failure to replicate -- the fedora
sysroot doesn't have dash.  As you say, the patch matches the kernel so,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

That said, what the hell is dash doing that relies on this?


r~

Re: [Qemu-devel] [PATCH for-2.12] linux-user/signal.c: Ensure AArch64 signal frame isn't too small

[Peter Maydell]

On 9 April 2018 at 23:05, Richard Henderson <rth@twiddle.net> wrote:
> On 04/10/2018 12:07 AM, Peter Maydell wrote:
>> In particular the dash shell
>> would segfault if the frame wasn't as big enough.
>
> Ah, that was the critical difference in my failure to replicate -- the fedora
> sysroot doesn't have dash.  As you say, the patch matches the kernel so,
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> That said, what the hell is dash doing that relies on this?

Yeah, I want to look more closely at what's going on here
tomorrow -- this is definitely a bug fix but I'm wondering
if it only masks a different underlying issue.

The spurious SEGV is the result of the call to
lock_user_struct() in target_setup_frame() failing
if we use too small a frame size, resulting in our
calling force_sigsegv().

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.12] linux-user/signal.c: Ensure AArch64 signal frame isn't too small

[Peter Maydell]

On 9 April 2018 at 23:05, Richard Henderson <rth@twiddle.net> wrote:
> On 04/10/2018 12:07 AM, Peter Maydell wrote:
>> In particular the dash shell
>> would segfault if the frame wasn't as big enough.
>
> Ah, that was the critical difference in my failure to replicate -- the fedora
> sysroot doesn't have dash.  As you say, the patch matches the kernel so,
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> That said, what the hell is dash doing that relies on this?

I figured out what's going on, and it's not particularly weird.

The memory map at the point where we take the signal is:
4000000000-4000001000 ---p 00000000 00:00 0
4000001000-4000801000 rw-p 00000000 00:00 0
4000801000-400081d000 r-xp 00000000 08:05 24190917
  /srv/chroot/xenial-aarch64/lib/aarch64-linux-gnu/ld-2.23.so

(conveniently guest_base == 0, so host and guest addresses are
identical).

Guest SP is 0x4000800590. Without this patch, we end up with
layout.total_size == 0x480 and frame_addr == 0x4000800110.

However, the range we try to lock for writing with
lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)
is always sizeof struct target_rt_sigframe, which is 0x1250.
So we try to lock from 0x4000800110 to 0x4000801360, which
extends beyond the rw ram block and into the read-only following
memory section which has ld-2.23.so in it.

So this patch fixes the problem for non-SVE guests
by bringing the size of the signal frame we allocate on the
stack back into line with the size of the lump of memory we
verify as being writable.

It does suggest that there's another bug here that will only
manifest if we're using SVE and end up with a larger signal
frame than the default -- we will in that case do the
lock_user on a lump of memory that's smaller than we're
actually going to try to write to, because it won't include
the extra part. Do we need to switch to using lock_user and
unlock_user and passing them layout.total_size, rather than
relying on the "lock size of this struct" functions?
(I think this is not a bugfix required for 2.12, because
nothing enables ARM_FEATURE_SVE yet, and without that feature
bit we won't ever create a signal frame that's larger than
sizeof(struct target_rt_sigframe).)

thanks
-- PMM