diff mbox

linux-user: Fix indirect syscall handling for MIPS

Message ID 20110805000549.GB5323@debian.localdomain
State New
Headers show

Commit Message

An-Cheng Huang Aug. 5, 2011, 12:05 a.m. UTC 
On Thu, Aug 04, 2011 at 11:43:31PM +0100, Peter Maydell wrote:
> On 4 August 2011 23:16, An-Cheng Huang <ancheng@ubnt.com> wrote:
> > A simpler approach would be to just change the number of arguments for
> > sys_syscall to 8 in the mips_syscall_args table so that for indirect
> > syscalls the "higher" arguments are always taken from the stack with
> > get_user_ual(). However, since there is a comment about "what to do
> > if get_user() fails", I don't know if this may cause breakage when the
> > arguments are not actually there? If someone can confirm that this is
> > harmless, the simple approach is probably better? Thanks.
> 
> In fact the Linux kernel will always read all four arguments off the
> stack for sys_syscall, regardless:
> http://lxr.linux.no/#linux+v3.0/arch/mips/kernel/scall32-o32.S#L188
> 
> So setting sys_syscall to 8 is not just easier but actually the Right
> Thing. The comment about get_user() is cut-n-paste from various other
> places in the file where it applies just as much -- it is no more of
> an issue for MIPS or for sys_syscall than for any other architecture
> or syscall. [ie it is a bug, but not in practice a very serious one,
> and you can ignore it for the purposes of fixing the bug you've found
> here.]
> 
> Incidentally, you can find the answer to the "what if get_user fails"
> question for MIPS here:
> http://lxr.linux.no/#linux+v3.0/arch/mips/kernel/scall32-o32.S#L166
> ...we should set ret to -TARGET_EFAULT and skip the call to do_syscall.

Ok the following patch changes the number of arguments for sys_syscall to 8 in mips_syscall_args and also skips the do_syscall() call if any of the get_user() calls fails. Do you think combining these makes sense or should they be two separate patches? Thanks.

Signed-off-by: An-Cheng Huang <ancheng@ubnt.com>
---
 linux-user/main.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

Comments

Peter Maydell Aug. 5, 2011, 9:27 a.m. UTC | #1 
On 5 August 2011 01:05, An-Cheng Huang <ancheng@ubnt.com> wrote:
> Ok the following patch changes the number of arguments for sys_syscall
> to 8 in mips_syscall_args and also skips the do_syscall() call if any
> of the get_user() calls fails. Do you think combining these makes sense
> or should they be two separate patches? Thanks.

The code in this patch looks good, but yes, I think they should
be two separate patches.

thanks
-- PMM
diff mbox

Patch

diff --git a/linux-user/main.c b/linux-user/main.c
index 6a8f4bd..701d96e 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -1669,7 +1669,7 @@  void cpu_loop(CPUPPCState *env)
 #define MIPS_SYS(name, args) args,
 
 static const uint8_t mips_syscall_args[] = {
-	MIPS_SYS(sys_syscall	, 0)	/* 4000 */
+	MIPS_SYS(sys_syscall	, 8)	/* 4000 */
 	MIPS_SYS(sys_exit	, 1)
 	MIPS_SYS(sys_fork	, 0)
 	MIPS_SYS(sys_read	, 3)
@@ -2090,11 +2090,22 @@  void cpu_loop(CPUMIPSState *env)
                 sp_reg = env->active_tc.gpr[29];
                 switch (nb_args) {
                 /* these arguments are taken from the stack */
-                /* FIXME - what to do if get_user() fails? */
-                case 8: get_user_ual(arg8, sp_reg + 28);
-                case 7: get_user_ual(arg7, sp_reg + 24);
-                case 6: get_user_ual(arg6, sp_reg + 20);
-                case 5: get_user_ual(arg5, sp_reg + 16);
+                case 8:
+                    if ((ret = get_user_ual(arg8, sp_reg + 28)) != 0) {
+                        goto done_syscall;
+                    }
+                case 7:
+                    if ((ret = get_user_ual(arg7, sp_reg + 24)) != 0) {
+                        goto done_syscall;
+                    }
+                case 6:
+                    if ((ret = get_user_ual(arg6, sp_reg + 20)) != 0) {
+                        goto done_syscall;
+                    }
+                case 5:
+                    if ((ret = get_user_ual(arg5, sp_reg + 16)) != 0) {
+                        goto done_syscall;
+                    }
                 default:
                     break;
                 }
@@ -2105,6 +2116,7 @@  void cpu_loop(CPUMIPSState *env)
                                  env->active_tc.gpr[7],
                                  arg5, arg6, arg7, arg8);
             }
+done_syscall:
             if (ret == -TARGET_QEMU_ESIGRETURN) {
                 /* Returning from a successful sigreturn syscall.
                    Avoid clobbering register state.  */