diff mbox

[29/43] linux-user: Fix broken m68k signal handling on 64 bit hosts

Message ID 1424814498-6993-30-git-send-email-mdroth@linux.vnet.ibm.com
State New
Headers show

Commit Message

Michael Roth Feb. 24, 2015, 9:48 p.m. UTC 
From: Peter Maydell <peter.maydell@linaro.org>

The m68k signal frame setup code which writes the signal return
trampoline code to the stack was assuming that a 'long' was 32 bits;
on 64 bit systems this meant we would end up writing the 32 bit
(2 insn) trampoline sequence to retaddr+4,retaddr+6 instead of
the intended retaddr+0,retaddr+2, resulting in a guest crash when
it tried to execute the invalid zero-bytes at retaddr+0.
Fix by using uint32_t instead; also use uint16_t rather than short
for consistency. This fixes bug LP:1404690.

Reported-by: Michel Boaventura
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
(cherry picked from commit 1669add752d9f29283f8ebf6a863d7b1e2d0f146)
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 linux-user/signal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Peter Maydell Feb. 25, 2015, 11:28 a.m. UTC | #1 
On 25 February 2015 at 17:39, Laurent Vivier <laurent@vivier.eu> wrote:
> Hi,
>
> I think you should use abi_long instead of  uint32_t.
>
> abi_long has an "aligned" attribute, and on m68k long are aligned on a short
> boundary.
>
>
> #ifdef TARGET_M68K
> #define ABI_INT_ALIGNMENT 2
> #define ABI_LONG_ALIGNMENT 2
> #define ABI_LLONG_ALIGNMENT 2
> #endif
>
> typedef uint32_t abi_ulong __attribute__((aligned(ABI_LONG_ALIGNMENT)));

In this instance it doesn't matter because get_sigframe() aligns
the pointer we're going to write the sigframe to, and the elements
in the struct before retcode[] won't make it worse than 4-aligned,
I think?

-- PMM
Peter Maydell Feb. 25, 2015, 12:14 p.m. UTC | #2 
On 25 February 2015 at 20:58, Laurent Vivier <laurent@vivier.eu> wrote:
>
>> Le 25 février 2015 à 12:28, Peter Maydell <peter.maydell@linaro.org> a
>> écrit :
>>
>>
>> On 25 February 2015 at 17:39, Laurent Vivier <laurent@vivier.eu> wrote:
>> > Hi,
>> >
>> > I think you should use abi_long instead of uint32_t.
>> >
>> > abi_long has an "aligned" attribute, and on m68k long are aligned on a
>> > short
>> > boundary.
>> >
>> >
>> > #ifdef TARGET_M68K
>> > #define ABI_INT_ALIGNMENT 2
>> > #define ABI_LONG_ALIGNMENT 2
>> > #define ABI_LLONG_ALIGNMENT 2
>> > #endif
>> >
>> > typedef uint32_t abi_ulong __attribute__((aligned(ABI_LONG_ALIGNMENT)));
>>
>> In this instance it doesn't matter because get_sigframe() aligns
>> the pointer we're going to write the sigframe to, and the elements
>> in the struct before retcode[] won't make it worse than 4-aligned,
>> I think?
>
> Yes, I agree. But the aim of the abi_* types is to define the target ABI.
> Thus, for consistency it should better to use the abi_long (or abi_ulong)
> instead of uint32_t.

Well, strictly speaking this is writing instructions into a
byte array rather than ABI longs, so it ought to do it
byte-at-a-time to respect the definition of the struct.
Anyway, if you want to submit a patch I don't object.

-- PMM
diff mbox

Patch

diff --git a/linux-user/signal.c b/linux-user/signal.c
index e11b208..a324fd1 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5091,7 +5091,7 @@  static void setup_frame(int sig, struct target_sigaction *ka,
     /* moveq #,d0; trap #0 */
 
     __put_user(0x70004e40 + (TARGET_NR_sigreturn << 16),
-                      (long *)(frame->retcode));
+                      (uint32_t *)(frame->retcode));
 
     /* Set up to return from userspace */
 
@@ -5225,8 +5225,8 @@  static void setup_rt_frame(int sig, struct target_sigaction *ka,
     /* moveq #,d0; notb d0; trap #0 */
 
     __put_user(0x70004600 + ((TARGET_NR_rt_sigreturn ^ 0xff) << 16),
-               (long *)(frame->retcode + 0));
-    __put_user(0x4e40, (short *)(frame->retcode + 4));
+               (uint32_t *)(frame->retcode + 0));
+    __put_user(0x4e40, (uint16_t *)(frame->retcode + 4));
 
     if (err)
         goto give_sigsegv;