[Qemu-devel] [PATCH] linux-user: Fix trampoline code for CRIS

[Qemu-devel] [PATCH] linux-user: Fix trampoline code for CRIS

[Stefan Weil]

__put_user can write bytes, words (2 bytes) or longwords (4 bytes).
Here obviously words should have been written, but bytes were written,
so values like 0x9c5f were truncated to 0x5f.

Fix this by changing retcode from uint8_t to to uint16_t in
target_signal_frame and also in the unused rt_signal_frame.

This problem was reported by static code analysis (smatch).

Cc: qemu-stable@nongnu.org
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

Please review this patch. I don't know details of the CRIS code
and cannot check my modification, so I don't know whether the new
code works as expected. Especially the byte order should be
checked.

Old and new code use tab characters, therefore checkpatch.pl
reports errors.

S. W.

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

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 01d7c39..697f46b 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -3659,7 +3659,7 @@ struct target_sigcontext {
 struct target_signal_frame {
         struct target_sigcontext sc;
         uint32_t extramask[TARGET_NSIG_WORDS - 1];
-        uint8_t retcode[8];       /* Trampoline code. */
+        uint16_t retcode[4];      /* Trampoline code. */
 };
 
 struct rt_signal_frame {
@@ -3667,7 +3667,7 @@ struct rt_signal_frame {
         void *puc;
         siginfo_t info;
         struct ucontext uc;
-        uint8_t retcode[8];       /* Trampoline code. */
+        uint16_t retcode[4];      /* Trampoline code. */
 };
 
 static void setup_sigcontext(struct target_sigcontext *sc, CPUCRISState *env)
@@ -3745,8 +3745,8 @@ static void setup_frame(int sig, struct target_sigaction *ka,
 	 */
 	err |= __put_user(0x9c5f, frame->retcode+0);
 	err |= __put_user(TARGET_NR_sigreturn, 
-			  frame->retcode+2);
-	err |= __put_user(0xe93d, frame->retcode+4);
+			  frame->retcode + 1);
+	err |= __put_user(0xe93d, frame->retcode + 2);
 
 	/* Save the mask.  */
 	err |= __put_user(set->sig[0], &frame->sc.oldmask);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] linux-user: Fix trampoline code for CRIS

[Peter Maydell]

On 1 February 2014 08:41, Stefan Weil <sw@weilnetz.de> wrote:
> __put_user can write bytes, words (2 bytes) or longwords (4 bytes).
> Here obviously words should have been written, but bytes were written,
> so values like 0x9c5f were truncated to 0x5f.
>
> Fix this by changing retcode from uint8_t to to uint16_t in
> target_signal_frame and also in the unused rt_signal_frame.

I believe this will do the right thing. The other possible approach
would be to do what the kernel does here (and what some of
the QEMU code for other targets does, eg x86) and put in the cast:

http://lxr.free-electrons.com/source/arch/cris/arch-v10/kernel/signal.c#L261

261                 /* This is movu.w __NR_sigreturn, r9; break 13; */
262                 err |= __put_user(0x9c5f,         (short
__user*)(frame->retcode+0));
263                 err |= __put_user(__NR_sigreturn, (short
__user*)(frame->retcode+2));
264                 err |= __put_user(0xe93d,         (short
__user*)(frame->retcode+4));

(obviously we'd want "(uint16_t *)").

Since CRIS looks (from a scan through its translate.c) like
a variable-width instruction set (in the sense that insns can
have immediate operands which might be 1/2/4 bytes long)
I think there's an argument here for following the kernel and
keeping retcode[] a byte array, for the implausible case where
we want to change the trampoline sequence to include an
insn with a 1 byte immediate value or something.

Either way I believe the endianness handling should be correct
since __put_user does host-to-target swapping for us.

It might be possible to test this by extracting some of the
userspace binaries from the cris system emulation test image
on the QEMU wiki (or it might not).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Fix trampoline code for CRIS

[Edgar E. Iglesias]

On Sat, Feb 01, 2014 at 12:09:06PM +0000, Peter Maydell wrote:
> On 1 February 2014 08:41, Stefan Weil <sw@weilnetz.de> wrote:
> > __put_user can write bytes, words (2 bytes) or longwords (4 bytes).
> > Here obviously words should have been written, but bytes were written,
> > so values like 0x9c5f were truncated to 0x5f.
> >
> > Fix this by changing retcode from uint8_t to to uint16_t in
> > target_signal_frame and also in the unused rt_signal_frame.
> 
> I believe this will do the right thing. The other possible approach
> would be to do what the kernel does here (and what some of
> the QEMU code for other targets does, eg x86) and put in the cast:
> 
> http://lxr.free-electrons.com/source/arch/cris/arch-v10/kernel/signal.c#L261
> 
> 261                 /* This is movu.w __NR_sigreturn, r9; break 13; */
> 262                 err |= __put_user(0x9c5f,         (short
> __user*)(frame->retcode+0));
> 263                 err |= __put_user(__NR_sigreturn, (short
> __user*)(frame->retcode+2));
> 264                 err |= __put_user(0xe93d,         (short
> __user*)(frame->retcode+4));
> 
> (obviously we'd want "(uint16_t *)").
> 
> Since CRIS looks (from a scan through its translate.c) like
> a variable-width instruction set (in the sense that insns can
> have immediate operands which might be 1/2/4 bytes long)
> I think there's an argument here for following the kernel and
> keeping retcode[] a byte array, for the implausible case where
> we want to change the trampoline sequence to include an
> insn with a 1 byte immediate value or something.
> 
> Either way I believe the endianness handling should be correct
> since __put_user does host-to-target swapping for us.
> 
> It might be possible to test this by extracting some of the
> userspace binaries from the cris system emulation test image
> on the QEMU wiki (or it might not).

Hi,

I've tested the patch, it works. CRIS insn stream is always 16bit
aligned, I think Stefans patch is OK.

Cheers,
Edgar

Re: [Qemu-devel] [PATCH] linux-user: Fix trampoline code for CRIS

[Peter Maydell]

On 2 February 2014 00:42, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Sat, Feb 01, 2014 at 12:09:06PM +0000, Peter Maydell wrote:
>> Since CRIS looks (from a scan through its translate.c) like
>> a variable-width instruction set (in the sense that insns can
>> have immediate operands which might be 1/2/4 bytes long)
>> I think there's an argument here for following the kernel and
>> keeping retcode[] a byte array, for the implausible case where
>> we want to change the trampoline sequence to include an
>> insn with a 1 byte immediate value or something.
>>
>> Either way I believe the endianness handling should be correct
>> since __put_user does host-to-target swapping for us.
>>
>> It might be possible to test this by extracting some of the
>> userspace binaries from the cris system emulation test image
>> on the QEMU wiki (or it might not).

> I've tested the patch, it works. CRIS insn stream is always 16bit
> aligned, I think Stefans patch is OK.

OK, if you're happy with this version I don't object to it
(it's just one of those 50/50 cases where my personal
taste would have tipped the other way).

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM