| Message ID | 1391244069-1538-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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
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);
__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(-)