[for-2.12] linux-user/signal.c: Put AArch64 frame record in the right place

AArch64 stack frames include a 'frame record' which holds a pointer
to the next frame record in the chain and the LR on entry to the
function. The procedure calling standard doesn't mandate where
exactly this frame record is in the stack frame, but for signal
frames the kernel puts it right at the top. We used to put it
there too, but in commit 7f0f4208b3a96f22 we accidentally put
the "enlarge to the 4K reserved space minimum" check after the
"allow for the frame record" code, rather than before it, with
the effect that the frame record would be inside the reserved
space and immediately after the last used part of it.

Move the frame record back out of the reserved space to where
we used to put it.

This bug shouldn't break any sensible guest code, but test
programs that deliberately look at the internal details
of the signal frame layout will not find what they are
expecting to see.

Fixes: 7f0f4208b3a96f22
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I'm marking this as for-2.12 on the basis that it puts our frame
layout back to exactly what 2.11 had, and so seems safest.
No sensible guest code should really care, though, so this is
in the "only if we're doing an rc4" bucket; but I think that the
softfloat fixes deserve an rc4 anyway.

 linux-user/signal.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

On 04/12/2018 04:02 AM, Peter Maydell wrote:
> AArch64 stack frames include a 'frame record' which holds a pointer
> to the next frame record in the chain and the LR on entry to the
> function. The procedure calling standard doesn't mandate where
> exactly this frame record is in the stack frame, but for signal
> frames the kernel puts it right at the top. We used to put it
> there too, but in commit 7f0f4208b3a96f22 we accidentally put
> the "enlarge to the 4K reserved space minimum" check after the
> "allow for the frame record" code, rather than before it, with
> the effect that the frame record would be inside the reserved
> space and immediately after the last used part of it.
> 
> Move the frame record back out of the reserved space to where
> we used to put it.
> 
> This bug shouldn't break any sensible guest code, but test
> programs that deliberately look at the internal details
> of the signal frame layout will not find what they are
> expecting to see.
> 
> Fixes: 7f0f4208b3a96f22
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> I'm marking this as for-2.12 on the basis that it puts our frame
> layout back to exactly what 2.11 had, and so seems safest.
> No sensible guest code should really care, though, so this is
> in the "only if we're doing an rc4" bucket; but I think that the
> softfloat fixes deserve an rc4 anyway.

Granted.


r~

Le 12/04/2018 à 16:02, Peter Maydell a écrit :
> AArch64 stack frames include a 'frame record' which holds a pointer
> to the next frame record in the chain and the LR on entry to the
> function. The procedure calling standard doesn't mandate where
> exactly this frame record is in the stack frame, but for signal
> frames the kernel puts it right at the top. We used to put it
> there too, but in commit 7f0f4208b3a96f22 we accidentally put
> the "enlarge to the 4K reserved space minimum" check after the
> "allow for the frame record" code, rather than before it, with
> the effect that the frame record would be inside the reserved
> space and immediately after the last used part of it.
> 
> Move the frame record back out of the reserved space to where
> we used to put it.
> 
> This bug shouldn't break any sensible guest code, but test
> programs that deliberately look at the internal details
> of the signal frame layout will not find what they are
> expecting to see.
> 
> Fixes: 7f0f4208b3a96f22
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I'm marking this as for-2.12 on the basis that it puts our frame
> layout back to exactly what 2.11 had, and so seems safest.
> No sensible guest code should really care, though, so this is
> in the "only if we're doing an rc4" bucket; but I think that the
> softfloat fixes deserve an rc4 anyway.
> 
>  linux-user/signal.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 8d9e6e8410..e6dfe0adfd 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1843,6 +1843,12 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
>          layout.total_size += sizeof(struct target_aarch64_ctx);
>      }
>  
> +    /* 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));
> +
>      /* Reserve space for the return code.  On a real system this would
>       * be within the VDSO.  So, despite the name this is not a "real"
>       * record within the frame.
> @@ -1850,12 +1856,6 @@ 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)) {
> 

It's hard to compare this code with the one in kernel, but if I compare
the offset of "fr" in QEMU and the one of "next_frame" they seem identical.

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

On 13 April 2018 at 15:18, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 12/04/2018 à 16:02, Peter Maydell a écrit :
>> @@ -1850,12 +1856,6 @@ 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)) {
>>
>
> It's hard to compare this code with the one in kernel, but if I compare
> the offset of "fr" in QEMU and the one of "next_frame" they seem identical.

Mmm. We're not quite the same as the kernel, because we keep 64 bits
of trampoline code in our target_rt_frame_record, which don't exist
on the stack in the kernel. So we'll never be exactly like the
kernel's layout (until/unless we implement a VDSO).

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

Thanks.

-- PMM

On 13 April 2018 at 05:46, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 04/12/2018 04:02 AM, Peter Maydell wrote:
>> AArch64 stack frames include a 'frame record' which holds a pointer
>> to the next frame record in the chain and the LR on entry to the
>> function. The procedure calling standard doesn't mandate where
>> exactly this frame record is in the stack frame, but for signal
>> frames the kernel puts it right at the top. We used to put it
>> there too, but in commit 7f0f4208b3a96f22 we accidentally put
>> the "enlarge to the 4K reserved space minimum" check after the
>> "allow for the frame record" code, rather than before it, with
>> the effect that the frame record would be inside the reserved
>> space and immediately after the last used part of it.
>>
>> Move the frame record back out of the reserved space to where
>> we used to put it.
>>
>> This bug shouldn't break any sensible guest code, but test
>> programs that deliberately look at the internal details
>> of the signal frame layout will not find what they are
>> expecting to see.
>>
>> Fixes: 7f0f4208b3a96f22
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> I'm marking this as for-2.12 on the basis that it puts our frame
>> layout back to exactly what 2.11 had, and so seems safest.
>> No sensible guest code should really care, though, so this is
>> in the "only if we're doing an rc4" bucket; but I think that the
>> softfloat fixes deserve an rc4 anyway.

We needed an rc4, so I've applied this to master.

-- PMM