From patchwork Sun Oct 14 19:52:27 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Maydell X-Patchwork-Id: 12217 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id A8CA623FB2 for ; Sun, 14 Oct 2012 19:52:34 +0000 (UTC) Received: from mail-ie0-f180.google.com (mail-ie0-f180.google.com [209.85.223.180]) by fiordland.canonical.com (Postfix) with ESMTP id 3211CA18AE2 for ; Sun, 14 Oct 2012 19:52:34 +0000 (UTC) Received: by mail-ie0-f180.google.com with SMTP id e10so6741874iej.11 for ; Sun, 14 Oct 2012 12:52:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-forwarded-to:x-forwarded-for:delivered-to:received-spf:from:to:cc :subject:date:message-id:x-mailer:x-gm-message-state; bh=vggs1IcliIY1vaz6/GywITl+s/yN5n4llobpr50jGos=; b=CAfdMLMNBBjbbMu7c/kQkMrRVg8qxDN1Bhw81nSYMsuXGAy2goT/5WRozDzwLzsBfR LqH+3wMnRNt3Enthv9qsJkVWz3bKeoWt8ZGXdoWMuRp7rqyCmdbm15TGUu72mWMWwC6m dTdPKD/fn1AFH0kGLZgtUpS2O+tnIsyStbDEkuxAxKy5SKXDmUX+WjjJqjYiK39bKwcI /eCcKdCTxviTVyzAF/UW0wUy7ydSYlhlIr0nww5dm2uPXcZldN/EaOj2IYn74nFqmvfC /jaqaT21xUVMKc/AtObgjatBTT9mV0GnZoicf2myQ5q2Pth1F4rjQEucB/o1SlsQheBW 1arQ== Received: by 10.50.87.134 with SMTP id ay6mr6939275igb.70.1350244353485; Sun, 14 Oct 2012 12:52:33 -0700 (PDT) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.50.67.148 with SMTP id n20csp485304igt; Sun, 14 Oct 2012 12:52:32 -0700 (PDT) Received: by 10.216.28.194 with SMTP id g44mr4986118wea.49.1350244351652; Sun, 14 Oct 2012 12:52:31 -0700 (PDT) Received: from mnementh.archaic.org.uk (1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.d.1.0.0.b.8.0.1.0.0.2.ip6.arpa. [2001:8b0:1d0::1]) by mx.google.com with ESMTPS id k29si14858647weo.59.2012.10.14.12.52.30 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 14 Oct 2012 12:52:31 -0700 (PDT) Received-SPF: neutral (google.com: 2001:8b0:1d0::1 is neither permitted nor denied by best guess record for domain of pm215@archaic.org.uk) client-ip=2001:8b0:1d0::1; Authentication-Results: mx.google.com; spf=neutral (google.com: 2001:8b0:1d0::1 is neither permitted nor denied by best guess record for domain of pm215@archaic.org.uk) smtp.mail=pm215@archaic.org.uk Received: from pm215 by mnementh.archaic.org.uk with local (Exim 4.72) (envelope-from ) id 1TNUEd-0004Ms-J1; Sun, 14 Oct 2012 20:52:27 +0100 From: Peter Maydell To: qemu-devel@nongnu.org Cc: patches@linaro.org, Blue Swirl Subject: [PATCH] arm-semi.c: Handle get/put_user() failure accessing arguments Date: Sun, 14 Oct 2012 20:52:27 +0100 Message-Id: <1350244347-16767-1-git-send-email-peter.maydell@linaro.org> X-Mailer: git-send-email 1.7.2.5 X-Gm-Message-State: ALoCoQnzLVQWHqO3Mg0wWOfC3hkNTNIDGswPBoasLRYeSU2vQfI3fsHsmTJraU1DAVJxrEgYvSu7 Rework the handling of arguments to ARM semihosting calls so that we handle a possible failure return from get_user_ual() or put_user_ual(). (This incidentally silences a lot of warnings from clang about "expression result unused"). Signed-off-by: Peter Maydell --- target-m68k/m68k-semi.c has a similar problem... target-arm/arm-semi.c | 167 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 106 insertions(+), 61 deletions(-) diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c index 73bde58..7743d67 100644 --- a/target-arm/arm-semi.c +++ b/target-arm/arm-semi.c @@ -166,17 +166,20 @@ static void arm_semi_flen_cb(CPUARMState *env, target_ulong ret, target_ulong er #endif } -#define ARG(n) \ -({ \ - target_ulong __arg; \ - /* FIXME - handle get_user() failure */ \ - get_user_ual(__arg, args + (n) * 4); \ - __arg; \ -}) +/* Read the input value from the argument block; fail the semihosting + * call if the memory read fails. + */ +#define GET_ARG(n) do { \ + if (get_user_ual(arg ## n, args + (n) * 4)) { \ + return (uint32_t)-1; \ + } \ +} while (0) + #define SET_ARG(n, val) put_user_ual(val, args + (n) * 4) uint32_t do_arm_semihosting(CPUARMState *env) { target_ulong args; + target_ulong arg0, arg1, arg2, arg3; char * s; int nr; uint32_t ret; @@ -191,33 +194,39 @@ uint32_t do_arm_semihosting(CPUARMState *env) args = env->regs[1]; switch (nr) { case TARGET_SYS_OPEN: - if (!(s = lock_user_string(ARG(0)))) + GET_ARG(0); + GET_ARG(1); + GET_ARG(2); + s = lock_user_string(arg0); + if (!s) { /* FIXME - should this error code be -TARGET_EFAULT ? */ return (uint32_t)-1; - if (ARG(1) >= 12) { - unlock_user(s, ARG(0), 0); + } + if (arg1 >= 12) { + unlock_user(s, arg0, 0); return (uint32_t)-1; } if (strcmp(s, ":tt") == 0) { - int result_fileno = ARG(1) < 4 ? STDIN_FILENO : STDOUT_FILENO; - unlock_user(s, ARG(0), 0); + int result_fileno = arg1 < 4 ? STDIN_FILENO : STDOUT_FILENO; + unlock_user(s, arg0, 0); return result_fileno; } if (use_gdb_syscalls()) { - gdb_do_syscall(arm_semi_cb, "open,%s,%x,1a4", ARG(0), - (int)ARG(2)+1, gdb_open_modeflags[ARG(1)]); + gdb_do_syscall(arm_semi_cb, "open,%s,%x,1a4", arg0, + (int)arg2+1, gdb_open_modeflags[arg1]); ret = env->regs[0]; } else { - ret = set_swi_errno(ts, open(s, open_modeflags[ARG(1)], 0644)); + ret = set_swi_errno(ts, open(s, open_modeflags[arg1], 0644)); } - unlock_user(s, ARG(0), 0); + unlock_user(s, arg0, 0); return ret; case TARGET_SYS_CLOSE: + GET_ARG(0); if (use_gdb_syscalls()) { - gdb_do_syscall(arm_semi_cb, "close,%x", ARG(0)); + gdb_do_syscall(arm_semi_cb, "close,%x", arg0); return env->regs[0]; } else { - return set_swi_errno(ts, close(ARG(0))); + return set_swi_errno(ts, close(arg0)); } case TARGET_SYS_WRITEC: { @@ -248,35 +257,45 @@ uint32_t do_arm_semihosting(CPUARMState *env) unlock_user(s, args, 0); return ret; case TARGET_SYS_WRITE: - len = ARG(2); + GET_ARG(0); + GET_ARG(1); + GET_ARG(2); + len = arg2; if (use_gdb_syscalls()) { arm_semi_syscall_len = len; - gdb_do_syscall(arm_semi_cb, "write,%x,%x,%x", ARG(0), ARG(1), len); + gdb_do_syscall(arm_semi_cb, "write,%x,%x,%x", arg0, arg1, len); return env->regs[0]; } else { - if (!(s = lock_user(VERIFY_READ, ARG(1), len, 1))) + s = lock_user(VERIFY_READ, arg1, len, 1); + if (!s) { /* FIXME - should this error code be -TARGET_EFAULT ? */ return (uint32_t)-1; - ret = set_swi_errno(ts, write(ARG(0), s, len)); - unlock_user(s, ARG(1), 0); + } + ret = set_swi_errno(ts, write(arg0, s, len)); + unlock_user(s, arg1, 0); if (ret == (uint32_t)-1) return -1; return len - ret; } case TARGET_SYS_READ: - len = ARG(2); + GET_ARG(0); + GET_ARG(1); + GET_ARG(2); + len = arg2; if (use_gdb_syscalls()) { arm_semi_syscall_len = len; - gdb_do_syscall(arm_semi_cb, "read,%x,%x,%x", ARG(0), ARG(1), len); + gdb_do_syscall(arm_semi_cb, "read,%x,%x,%x", arg0, arg1, len); return env->regs[0]; } else { - if (!(s = lock_user(VERIFY_WRITE, ARG(1), len, 0))) + s = lock_user(VERIFY_WRITE, arg1, len, 0); + if (!s) { /* FIXME - should this error code be -TARGET_EFAULT ? */ return (uint32_t)-1; - do - ret = set_swi_errno(ts, read(ARG(0), s, len)); - while (ret == -1 && errno == EINTR); - unlock_user(s, ARG(1), len); + } + do { + ret = set_swi_errno(ts, read(arg0, s, len)); + } while (ret == -1 && errno == EINTR); + unlock_user(s, arg1, len); if (ret == (uint32_t)-1) return -1; return len - ret; @@ -285,30 +304,34 @@ uint32_t do_arm_semihosting(CPUARMState *env) /* XXX: Read from debug console. Not implemented. */ return 0; case TARGET_SYS_ISTTY: + GET_ARG(0); if (use_gdb_syscalls()) { - gdb_do_syscall(arm_semi_cb, "isatty,%x", ARG(0)); + gdb_do_syscall(arm_semi_cb, "isatty,%x", arg0); return env->regs[0]; } else { - return isatty(ARG(0)); + return isatty(arg0); } case TARGET_SYS_SEEK: + GET_ARG(0); + GET_ARG(1); if (use_gdb_syscalls()) { - gdb_do_syscall(arm_semi_cb, "lseek,%x,%x,0", ARG(0), ARG(1)); + gdb_do_syscall(arm_semi_cb, "lseek,%x,%x,0", arg0, arg1); return env->regs[0]; } else { - ret = set_swi_errno(ts, lseek(ARG(0), ARG(1), SEEK_SET)); + ret = set_swi_errno(ts, lseek(arg0, arg1, SEEK_SET)); if (ret == (uint32_t)-1) return -1; return 0; } case TARGET_SYS_FLEN: + GET_ARG(0); if (use_gdb_syscalls()) { gdb_do_syscall(arm_semi_flen_cb, "fstat,%x,%x", - ARG(0), env->regs[13]-64); + arg0, env->regs[13]-64); return env->regs[0]; } else { struct stat buf; - ret = set_swi_errno(ts, fstat(ARG(0), &buf)); + ret = set_swi_errno(ts, fstat(arg0, &buf)); if (ret == (uint32_t)-1) return -1; return buf.st_size; @@ -317,35 +340,43 @@ uint32_t do_arm_semihosting(CPUARMState *env) /* XXX: Not implemented. */ return -1; case TARGET_SYS_REMOVE: + GET_ARG(0); + GET_ARG(1); if (use_gdb_syscalls()) { - gdb_do_syscall(arm_semi_cb, "unlink,%s", ARG(0), (int)ARG(1)+1); + gdb_do_syscall(arm_semi_cb, "unlink,%s", arg0, (int)arg1+1); ret = env->regs[0]; } else { - if (!(s = lock_user_string(ARG(0)))) + s = lock_user_string(arg0); + if (!s) { /* FIXME - should this error code be -TARGET_EFAULT ? */ return (uint32_t)-1; + } ret = set_swi_errno(ts, remove(s)); - unlock_user(s, ARG(0), 0); + unlock_user(s, arg0, 0); } return ret; case TARGET_SYS_RENAME: + GET_ARG(0); + GET_ARG(1); + GET_ARG(2); + GET_ARG(3); if (use_gdb_syscalls()) { gdb_do_syscall(arm_semi_cb, "rename,%s,%s", - ARG(0), (int)ARG(1)+1, ARG(2), (int)ARG(3)+1); + arg0, (int)arg1+1, arg2, (int)arg3+1); return env->regs[0]; } else { char *s2; - s = lock_user_string(ARG(0)); - s2 = lock_user_string(ARG(2)); + s = lock_user_string(arg0); + s2 = lock_user_string(arg2); if (!s || !s2) /* FIXME - should this error code be -TARGET_EFAULT ? */ ret = (uint32_t)-1; else ret = set_swi_errno(ts, rename(s, s2)); if (s2) - unlock_user(s2, ARG(2), 0); + unlock_user(s2, arg2, 0); if (s) - unlock_user(s, ARG(0), 0); + unlock_user(s, arg0, 0); return ret; } case TARGET_SYS_CLOCK: @@ -353,15 +384,19 @@ uint32_t do_arm_semihosting(CPUARMState *env) case TARGET_SYS_TIME: return set_swi_errno(ts, time(NULL)); case TARGET_SYS_SYSTEM: + GET_ARG(0); + GET_ARG(1); if (use_gdb_syscalls()) { - gdb_do_syscall(arm_semi_cb, "system,%s", ARG(0), (int)ARG(1)+1); + gdb_do_syscall(arm_semi_cb, "system,%s", arg0, (int)arg1+1); return env->regs[0]; } else { - if (!(s = lock_user_string(ARG(0)))) + s = lock_user_string(arg0); + if (!s) { /* FIXME - should this error code be -TARGET_EFAULT ? */ return (uint32_t)-1; + } ret = set_swi_errno(ts, system(s)); - unlock_user(s, ARG(0), 0); + unlock_user(s, arg0, 0); return ret; } case TARGET_SYS_ERRNO: @@ -375,22 +410,24 @@ uint32_t do_arm_semihosting(CPUARMState *env) /* Build a command-line from the original argv. * * The inputs are: - * * ARG(0), pointer to a buffer of at least the size - * specified in ARG(1). - * * ARG(1), size of the buffer pointed to by ARG(0) in + * * arg0, pointer to a buffer of at least the size + * specified in arg1. + * * arg1, size of the buffer pointed to by arg0 in * bytes. * * The outputs are: - * * ARG(0), pointer to null-terminated string of the + * * arg0, pointer to null-terminated string of the * command line. - * * ARG(1), length of the string pointed to by ARG(0). + * * arg1, length of the string pointed to by arg0. */ char *output_buffer; - size_t input_size = ARG(1); + size_t input_size; size_t output_size; int status = 0; - + GET_ARG(0); + GET_ARG(1); + input_size = arg1; /* Compute the size of the output string. */ #if !defined(CONFIG_USER_ONLY) output_size = strlen(ts->boot_info->kernel_filename) @@ -414,10 +451,13 @@ uint32_t do_arm_semihosting(CPUARMState *env) } /* Adjust the command-line length. */ - SET_ARG(1, output_size - 1); + if (SET_ARG(1, output_size - 1)) { + /* Couldn't write back to argument block */ + return -1; + } /* Lock the buffer on the ARM side. */ - output_buffer = lock_user(VERIFY_WRITE, ARG(0), output_size, 0); + output_buffer = lock_user(VERIFY_WRITE, arg0, output_size, 0); if (!output_buffer) { return -1; } @@ -449,7 +489,7 @@ uint32_t do_arm_semihosting(CPUARMState *env) out: #endif /* Unlock the buffer on the ARM side. */ - unlock_user(output_buffer, ARG(0), output_size); + unlock_user(output_buffer, arg0, output_size); return status; } @@ -457,6 +497,7 @@ uint32_t do_arm_semihosting(CPUARMState *env) { uint32_t *ptr; uint32_t limit; + GET_ARG(0); #ifdef CONFIG_USER_ONLY /* Some C libraries assume the heap immediately follows .bss, so @@ -477,25 +518,29 @@ uint32_t do_arm_semihosting(CPUARMState *env) ts->heap_limit = limit; } - if (!(ptr = lock_user(VERIFY_WRITE, ARG(0), 16, 0))) + ptr = lock_user(VERIFY_WRITE, arg0, 16, 0); + if (!ptr) { /* FIXME - should this error code be -TARGET_EFAULT ? */ return (uint32_t)-1; + } ptr[0] = tswap32(ts->heap_base); ptr[1] = tswap32(ts->heap_limit); ptr[2] = tswap32(ts->stack_base); ptr[3] = tswap32(0); /* Stack limit. */ - unlock_user(ptr, ARG(0), 16); + unlock_user(ptr, arg0, 16); #else limit = ram_size; - if (!(ptr = lock_user(VERIFY_WRITE, ARG(0), 16, 0))) + ptr = lock_user(VERIFY_WRITE, arg0, 16, 0); + if (!ptr) { /* FIXME - should this error code be -TARGET_EFAULT ? */ return (uint32_t)-1; + } /* TODO: Make this use the limit of the loaded application. */ ptr[0] = tswap32(limit / 2); ptr[1] = tswap32(limit); ptr[2] = tswap32(limit); /* Stack base */ ptr[3] = tswap32(0); /* Stack limit. */ - unlock_user(ptr, ARG(0), 16); + unlock_user(ptr, arg0, 16); #endif return 0; }