[Qemu-devel] [PATCH 0/8] linux-user: fix various coverity nits

[Qemu-devel] [PATCH 0/8] linux-user: fix various coverity nits

[Peter Maydell]

This patchset fixes a bunch of minor bugs spotted by Coverity.

(With these I think the only remaining Coverity issue
for linux-user is that it would prefer us not to call rand().
That is awkward to fix because we can't rely on /dev/urandom
existing inside chroot environments where QEMU might be
deployed.)

thanks
-- PMM

Peter Maydell (8):
  linux-user: Pass missing MAP_ANONYMOUS to target_mmap() call
  linux-user: Check lock_user() return value for NULL
  linux-user: Fix incorrect use of host errno in do_ioctl_dm()
  linux-user: Fix error handling in flatload.c target_pread()
  linux-user: Don't write off end of new_utsname buffer
  linux-user: Check dump_write() return in elf_core_dump()
  linux-user: Use glib malloc functions in load_symbols()
  linux-user: Fix memchr() argument in open_self_cmdline()

 linux-user/elfload.c  | 20 +++++++++++---------
 linux-user/flatload.c |  6 ++++++
 linux-user/syscall.c  | 21 ++++++++++++++++-----
 3 files changed, 33 insertions(+), 14 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/8] linux-user: Pass missing MAP_ANONYMOUS to target_mmap() call

[Peter Maydell]

A target_mmap() call in load_elf_binary() was missing the MAP_ANONYMOUS
flag. (Spotted by Coverity, because target_mmap() will try to use
-1 as the filedescriptor in this case.)

This has never been noticed because the code in question is for
handling ancient SVr4 iBCS2 binaries.

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

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f807baf..38e210e 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2233,7 +2233,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
                we do not have the power to recompile these, we emulate
                the SVr4 behavior.  Sigh.  */
             target_mmap(0, qemu_host_page_size, PROT_READ | PROT_EXEC,
-                        MAP_FIXED | MAP_PRIVATE, -1, 0);
+                        MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
         }
     }
 
-- 
1.9.1

[Qemu-devel] [PATCH 2/8] linux-user: Check lock_user() return value for NULL

[Peter Maydell]

lock_user() can return NULL, which typically means the syscall
should fail with EFAULT. Add checks in various places where
Coverity spotted that we were missing them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2e71879..b868fb9 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4512,6 +4512,11 @@ static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
     host_data = (char*)host_dm + host_dm->data_start;
 
     argptr = lock_user(VERIFY_READ, guest_data, guest_data_size, 1);
+    if (!argptr) {
+        ret = -TARGET_EFAULT;
+        goto out;
+    }
+
     switch (ie->host_cmd) {
     case DM_REMOVE_ALL:
     case DM_LIST_DEVICES:
@@ -10768,6 +10773,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 
     case TARGET_NR_mq_unlink:
         p = lock_user_string(arg1 - 1);
+        if (!p) {
+            ret = -TARGET_EFAULT;
+            break;
+        }
         ret = get_errno(mq_unlink(p));
         unlock_user (p, arg1, 0);
         break;
-- 
1.9.1

[Qemu-devel] [PATCH 3/8] linux-user: Fix incorrect use of host errno in do_ioctl_dm()

[Peter Maydell]

do_ioctl_dm() should return target errno values, not host ones;
correct an accidental use of a host errno in an error path.

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

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b868fb9..37d26bb 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4505,7 +4505,7 @@ static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
 
     guest_data = arg + host_dm->data_start;
     if ((guest_data - arg) < 0) {
-        ret = -EINVAL;
+        ret = -TARGET_EINVAL;
         goto out;
     }
     guest_data_size = host_dm->data_size - host_dm->data_start;
-- 
1.9.1

[Qemu-devel] [PATCH 4/8] linux-user: Fix error handling in flatload.c target_pread()

[Peter Maydell]

The flatload.c target_pread() function is supposed to return
0 on success or negative host errnos; however it wasn't
checking lock_user() for failure or returning the errno from
the pread() call. Fix these problems (the first of which is
noted by Coverity).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/flatload.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index 48ad1c5..99492d3 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -95,7 +95,13 @@ static int target_pread(int fd, abi_ulong ptr, abi_ulong len,
     int ret;
 
     buf = lock_user(VERIFY_WRITE, ptr, len, 0);
+    if (!buf) {
+        return -EFAULT;
+    }
     ret = pread(fd, buf, len, offset);
+    if (ret < 0) {
+        ret = -errno;
+    }
     unlock_user(buf, ptr, len);
     return ret;
 }
-- 
1.9.1

[Qemu-devel] [PATCH 5/8] linux-user: Don't write off end of new_utsname buffer

[Peter Maydell]

Use g_strlcpy() rather than strcpy() to copy the uname string
into the structure we return to the guest for the uname syscall.
This avoids overrunning the buffer if the user passed us an
overlong string via the QEMU command line.

We fix a comment typo while we're in the neighbourhood.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 37d26bb..f849a5d 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8911,12 +8911,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 goto efault;
             ret = get_errno(sys_uname(buf));
             if (!is_error(ret)) {
-                /* Overrite the native machine name with whatever is being
+                /* Overwrite the native machine name with whatever is being
                    emulated. */
                 strcpy (buf->machine, cpu_to_uname_machine(cpu_env));
                 /* Allow the user to override the reported release.  */
-                if (qemu_uname_release && *qemu_uname_release)
-                  strcpy (buf->release, qemu_uname_release);
+                if (qemu_uname_release && *qemu_uname_release) {
+                    g_strlcpy(buf->release, qemu_uname_release,
+                              sizeof(buf->release));
+                }
             }
             unlock_user_struct(buf, arg1, 1);
         }
-- 
1.9.1

[Qemu-devel] [PATCH 6/8] linux-user: Check dump_write() return in elf_core_dump()

[Peter Maydell]

One of the calls to dump_write() in elf_core_dump() was missing
a check for failure (spotted by Coverity). Add the check to
bring it into line with the other calls from this function.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/elfload.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 38e210e..7c46cfb 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -3053,7 +3053,9 @@ static int elf_core_dump(int signr, const CPUArchState *env)
         phdr.p_align = ELF_EXEC_PAGESIZE;
 
         bswap_phdr(&phdr, 1);
-        dump_write(fd, &phdr, sizeof (phdr));
+        if (dump_write(fd, &phdr, sizeof(phdr)) != 0) {
+            goto out;
+        }
     }
 
     /*
-- 
1.9.1

[Qemu-devel] [PATCH 7/8] linux-user: Use glib malloc functions in load_symbols()

[Peter Maydell]

Switch to using the glib malloc functions in load_symbols();
this deals with a Coverity complaint about possible
integer overflow calculating the allocation size with
'nsyms * sizeof(*syms)'.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I opted to use the _try_ versions rather than switching
to the abort-on-failure allocation functions because
(a) the handle-failure code is already in place and correct
(b) loading symbols from the ELF file is debug-only and
can safely be skipped
---
 linux-user/elfload.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 7c46cfb..b062199 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2111,19 +2111,19 @@ static void load_symbols(struct elfhdr *hdr, int fd, abi_ulong load_bias)
 
  found:
     /* Now know where the strtab and symtab are.  Snarf them.  */
-    s = malloc(sizeof(*s));
+    s = g_try_new(struct syminfo, 1);
     if (!s) {
         goto give_up;
     }
 
     i = shdr[str_idx].sh_size;
-    s->disas_strtab = strings = malloc(i);
+    s->disas_strtab = strings = g_try_malloc(i);
     if (!strings || pread(fd, strings, i, shdr[str_idx].sh_offset) != i) {
         goto give_up;
     }
 
     i = shdr[sym_idx].sh_size;
-    syms = malloc(i);
+    syms = g_try_malloc(i);
     if (!syms || pread(fd, syms, i, shdr[sym_idx].sh_offset) != i) {
         goto give_up;
     }
@@ -2157,7 +2157,7 @@ static void load_symbols(struct elfhdr *hdr, int fd, abi_ulong load_bias)
        that we threw away.  Whether or not this has any effect on the
        memory allocation depends on the malloc implementation and how
        many symbols we managed to discard.  */
-    new_syms = realloc(syms, nsyms * sizeof(*syms));
+    new_syms = g_try_renew(struct elf_sym, syms, nsyms);
     if (new_syms == NULL) {
         goto give_up;
     }
@@ -2178,9 +2178,9 @@ static void load_symbols(struct elfhdr *hdr, int fd, abi_ulong load_bias)
     return;
 
 give_up:
-    free(s);
-    free(strings);
-    free(syms);
+    g_free(s);
+    g_free(strings);
+    g_free(syms);
 }
 
 int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
-- 
1.9.1

[Qemu-devel] [PATCH 8/8] linux-user: Fix memchr() argument in open_self_cmdline()

[Peter Maydell]

In open_self_cmdline() we look for a 0 in the buffer we read
from /prc/self/cmdline. We were incorrectly passing the length
of our buf[] array to memchr() as the length to search, rather
than the number of bytes we actually read into it, which could
be shorter. This was spotted by Coverity (because it could
result in our trying to pass a negative length argument to
write()).

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

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f849a5d..9dbd711 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6530,7 +6530,7 @@ static int open_self_cmdline(void *cpu_env, int fd)
         if (!word_skipped) {
             /* Skip the first string, which is the path to qemu-*-static
                instead of the actual command. */
-            cp_buf = memchr(buf, 0, sizeof(buf));
+            cp_buf = memchr(buf, 0, nb_read);
             if (cp_buf) {
                 /* Null byte found, skip one string */
                 cp_buf++;
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/8] linux-user: fix various coverity nits

[Paolo Bonzini]

On 12/07/2016 14:02, Peter Maydell wrote:
> (With these I think the only remaining Coverity issue
> for linux-user is that it would prefer us not to call rand().
> That is awkward to fix because we can't rely on /dev/urandom
> existing inside chroot environments where QEMU might be
> deployed.)

We can use getrandom perhaps if available.

Paolo