[PATCH 0/4] semihosting: fix various coverity issues

[PATCH 0/4] semihosting: fix various coverity issues

[Peter Maydell]

This patchset fixes a handful of bugs in the semihosting code
noticed by Coverity.

thanks
-- PMM

Peter Maydell (4):
  semihosting: Don't return negative values on
    qemu_semihosting_console_write() failure
  semihosting: Don't copy buffer after console_write()
  semihosting: Check for errors on SET_ARG()
  semihosting: Fix handling of buffer in TARGET_SYS_TMPNAM

 semihosting/arm-compat-semi.c | 29 ++++++++++++++++++++++++-----
 semihosting/console.c         |  3 ++-
 semihosting/syscalls.c        |  2 +-
 3 files changed, 27 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/4] semihosting: Don't return negative values on qemu_semihosting_console_write() failure

[Peter Maydell]

The documentation comment for qemu_semihosting_console_write() says
 * Returns: number of bytes written -- this should only ever be short
 * on some sort of i/o error.

and the callsites rely on this.  However, the implementation code
path which sends console output to a chardev doesn't honour this,
and will return negative values on error.  Bring it into line with
the other implementation codepaths and the documentation, so that
it returns 0 on error.

Spotted by Coverity, because console_write() passes the return value
to unlock_user(), which doesn't accept a negative length.

Resolves: Coverity CID 1490288
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
console_write() doesn't need to pass the length to unlock_user()
at all, as it happens -- see the next patch.
---
 semihosting/console.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/semihosting/console.c b/semihosting/console.c
index 5b1ec0a1c39..0f976fe8cb1 100644
--- a/semihosting/console.c
+++ b/semihosting/console.c
@@ -111,7 +111,8 @@ int qemu_semihosting_console_read(CPUState *cs, void *buf, int len)
 int qemu_semihosting_console_write(void *buf, int len)
 {
     if (console.chr) {
-        return qemu_chr_write_all(console.chr, (uint8_t *)buf, len);
+        int r = qemu_chr_write_all(console.chr, (uint8_t *)buf, len);
+        return r < 0 ? 0 : r;
     } else {
         return fwrite(buf, 1, len, stderr);
     }
-- 
2.25.1

Re: [PATCH 1/4] semihosting: Don't return negative values on qemu_semihosting_console_write() failure

[Richard Henderson]

On 7/19/22 17:41, Peter Maydell wrote:
> The documentation comment for qemu_semihosting_console_write() says
>   * Returns: number of bytes written -- this should only ever be short
>   * on some sort of i/o error.
> 
> and the callsites rely on this.  However, the implementation code
> path which sends console output to a chardev doesn't honour this,
> and will return negative values on error.  Bring it into line with
> the other implementation codepaths and the documentation, so that
> it returns 0 on error.
> 
> Spotted by Coverity, because console_write() passes the return value
> to unlock_user(), which doesn't accept a negative length.
> 
> Resolves: Coverity CID 1490288
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> console_write() doesn't need to pass the length to unlock_user()
> at all, as it happens -- see the next patch.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/4] semihosting: Don't copy buffer after console_write()

[Peter Maydell]

The console_write() semihosting function outputs guest data from a
buffer; it doesn't update that buffer.  It therefore doesn't need to
pass a length value to unlock_user(), but can pass 0, meaning "do not
copy any data back to the guest memory".

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 semihosting/syscalls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/semihosting/syscalls.c b/semihosting/syscalls.c
index 4847f66c023..508a0ad88c6 100644
--- a/semihosting/syscalls.c
+++ b/semihosting/syscalls.c
@@ -627,7 +627,7 @@ static void console_write(CPUState *cs, gdb_syscall_complete_cb complete,
     }
     ret = qemu_semihosting_console_write(ptr, len);
     complete(cs, ret ? ret : -1, ret ? 0 : EIO);
-    unlock_user(ptr, buf, ret);
+    unlock_user(ptr, buf, 0);
 }
 
 static void console_fstat(CPUState *cs, gdb_syscall_complete_cb complete,
-- 
2.25.1

Re: [PATCH 2/4] semihosting: Don't copy buffer after console_write()

[Richard Henderson]

On 7/19/22 17:41, Peter Maydell wrote:
> The console_write() semihosting function outputs guest data from a
> buffer; it doesn't update that buffer.  It therefore doesn't need to
> pass a length value to unlock_user(), but can pass 0, meaning "do not
> copy any data back to the guest memory".
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   semihosting/syscalls.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 3/4] semihosting: Check for errors on SET_ARG()

[Peter Maydell]

The SET_ARG() macro returns an error indication; we check this in the
TARGET_SYS_GET_CMDLINE case but not when we use it in implementing
TARGET_SYS_ELAPSED.  Check for and handle the errors via the do_fault
codepath, and update the comment documenting the SET_ARG() and
GET_ARG() macros to note how they handle memory access errors.

Resolves: Coverity CID 1490287
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 semihosting/arm-compat-semi.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 1a1e2a69605..d12288fc806 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -171,6 +171,12 @@ static LayoutInfo common_semi_find_bases(CPUState *cs)
  * Read the input value from the argument block; fail the semihosting
  * call if the memory read fails. Eventually we could use a generic
  * CPUState helper function here.
+ * Note that GET_ARG() handles memory access errors by jumping to
+ * do_fault, so must be used as the first thing done in handling a
+ * semihosting call, to avoid accidentally leaking allocated resources.
+ * SET_ARG(), since it unavoidably happens late, instead returns an
+ * error indication (0 on success, non-0 for error) which the caller
+ * should check.
  */
 
 #define GET_ARG(n) do {                                 \
@@ -739,10 +745,14 @@ void do_common_semihosting(CPUState *cs)
     case TARGET_SYS_ELAPSED:
         elapsed = get_clock() - clock_start;
         if (sizeof(target_ulong) == 8) {
-            SET_ARG(0, elapsed);
+            if (SET_ARG(0, elapsed)) {
+                goto do_fault;
+            }
         } else {
-            SET_ARG(0, (uint32_t) elapsed);
-            SET_ARG(1, (uint32_t) (elapsed >> 32));
+            if (SET_ARG(0, (uint32_t) elapsed) ||
+                SET_ARG(1, (uint32_t) (elapsed >> 32))) {
+                goto do_fault;
+            }
         }
         common_semi_set_ret(cs, 0);
         break;
-- 
2.25.1

Re: [PATCH 3/4] semihosting: Check for errors on SET_ARG()

[Richard Henderson]

On 7/19/22 17:41, Peter Maydell wrote:
> The SET_ARG() macro returns an error indication; we check this in the
> TARGET_SYS_GET_CMDLINE case but not when we use it in implementing
> TARGET_SYS_ELAPSED.  Check for and handle the errors via the do_fault
> codepath, and update the comment documenting the SET_ARG() and
> GET_ARG() macros to note how they handle memory access errors.
> 
> Resolves: Coverity CID 1490287
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/4] semihosting: Fix handling of buffer in TARGET_SYS_TMPNAM

[Peter Maydell]

The TARGET_SYS_TMPNAM implementation has two bugs spotted by
Coverity:
 * confusion about whether 'len' has the length of the string
   including or excluding the terminating NUL means we
   lock_user() len bytes of memory but memcpy() len + 1 bytes
 * In the error-exit cases we forget to free() the buffer
   that asprintf() returned to us

Resolves: Coverity CID 1490285, 1490289
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 semihosting/arm-compat-semi.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index d12288fc806..e741674238f 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -504,16 +504,25 @@ void do_common_semihosting(CPUState *cs)
         GET_ARG(1);
         GET_ARG(2);
         len = asprintf(&s, "/tmp/qemu-%x%02x", getpid(), (int)arg1 & 0xff);
+        if (len < 0) {
+            common_semi_set_ret(cs, -1);
+            break;
+        }
+
+        /* Allow for trailing NUL */
+        len++;
         /* Make sure there's enough space in the buffer */
-        if (len < 0 || len >= arg2) {
+        if (len > arg2) {
+            free(s);
             common_semi_set_ret(cs, -1);
             break;
         }
         p = lock_user(VERIFY_WRITE, arg0, len, 0);
         if (!p) {
+            free(s);
             goto do_fault;
         }
-        memcpy(p, s, len + 1);
+        memcpy(p, s, len);
         unlock_user(p, arg0, len);
         free(s);
         common_semi_set_ret(cs, 0);
-- 
2.25.1

Re: [PATCH 4/4] semihosting: Fix handling of buffer in TARGET_SYS_TMPNAM

[Richard Henderson]

On 7/19/22 17:41, Peter Maydell wrote:
> The TARGET_SYS_TMPNAM implementation has two bugs spotted by
> Coverity:
>   * confusion about whether 'len' has the length of the string
>     including or excluding the terminating NUL means we
>     lock_user() len bytes of memory but memcpy() len + 1 bytes
>   * In the error-exit cases we forget to free() the buffer
>     that asprintf() returned to us
> 
> Resolves: Coverity CID 1490285, 1490289
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   semihosting/arm-compat-semi.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 0/4] semihosting: fix various coverity issues

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> This patchset fixes a handful of bugs in the semihosting code
> noticed by Coverity.

Queued to testing/next, thanks.

-- 
Alex Bennée