On 9 April 2018 at 23:05, Richard Henderson <r...@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.hender...@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