Re: [Qemu-devel] [PATCH for-2.12] linux-user/signal.c: Ensure AArch64 signal frame isn't too small
On 9 April 2018 at 23:05, Richard Henderson 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 > > 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: 40-401000 ---p 00:00 0 401000-4000801000 rw-p 00:00 0 4000801000-400081d000 r-xp 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
Re: [Qemu-devel] [PATCH for-2.12] linux-user/signal.c: Ensure AArch64 signal frame isn't too small
On 9 April 2018 at 23:05, Richard Henderson 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 > > 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
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 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
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 > Fixes: https://bugs.launchpad.net/bugs/1761535 > Fixes: 8c5931de0ac77388096d79ceb > Signed-off-by: Peter Maydell > --- > 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