1. Patchew
  2. QEMU
  3. View series

[PATCH] linux-user: Fix Coverity CID 1430271 / CID 1430272

Laurent Vivier posted 1 patch 3 years, 9 months ago
Test checkpatch passed
Test docker-mingw@fedora passed
Test FreeBSD passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200709200010.1016741-1-laurent@vivier.eu
Maintainers: Laurent Vivier <laurent@vivier.eu>
linux-user/strace.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
[PATCH] linux-user: Fix Coverity CID 1430271 / CID 1430272
Posted by Laurent Vivier 3 years, 9 months ago 
In new functions print_ioctl() and print_syscall_ret_ioctl(), we don't
check if lock_user() returns NULL and this would cause a segfault in
thunk_print().

If lock_user() returns NULL don't call thunk_print() but prints only the
value of the (invalid) pointer.

Tested with:

    # cat ioctl.c
    #include <unistd.h>
    #include <sys/ioctl.h>

    int main(void)
    {
        int ret;

        ret = ioctl(STDOUT_FILENO, TCGETS, 0xdeadbeef);
        ret = ioctl(STDOUT_FILENO, TCSETSF, 0xdeadbeef);
        return 0;
    }
    # QEMU_STRACE= ./ioctl
    ...
    578 ioctl(1,TCGETS,0xdeadbeef) = -1 errno=2 (Bad address)
    578 ioctl(1,TCSETSF,0xdeadbeef) = -1 errno=2 (Bad address)
    ...
    # QEMU_STRACE= passwd
    ...
    623 ioctl(0,TCGETS,0x3fffed04) = 0 ({})
    623 ioctl(0,TCSETSF,{}) = 0
    ...

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: 79482e5987c8 ("linux-user: Add strace support for printing arguments of ioctl()")
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/strace.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index 5235b2260cdd..39554d903911 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -889,8 +889,12 @@ print_syscall_ret_ioctl(const struct syscallname *name, abi_long ret,
             arg_type++;
             target_size = thunk_type_size(arg_type, 0);
             argptr = lock_user(VERIFY_READ, arg2, target_size, 1);
-            thunk_print(argptr, arg_type);
-            unlock_user(argptr, arg2, target_size);
+            if (argptr) {
+                thunk_print(argptr, arg_type);
+                unlock_user(argptr, arg2, target_size);
+            } else {
+                print_pointer(arg2, 1);
+            }
             qemu_log(")");
         }
     }
@@ -3119,8 +3123,12 @@ print_ioctl(const struct syscallname *name,
                     arg_type++;
                     target_size = thunk_type_size(arg_type, 0);
                     argptr = lock_user(VERIFY_READ, arg2, target_size, 1);
-                    thunk_print(argptr, arg_type);
-                    unlock_user(argptr, arg2, target_size);
+                    if (argptr) {
+                        thunk_print(argptr, arg_type);
+                        unlock_user(argptr, arg2, target_size);
+                    } else {
+                        print_pointer(arg2, 1);
+                    }
                     break;
                 }
                 break;
-- 
2.26.2
Re: [PATCH] linux-user: Fix Coverity CID 1430271 / CID 1430272
Posted by Peter Maydell 3 years, 9 months ago 
On Thu, 9 Jul 2020 at 21:00, Laurent Vivier <laurent@vivier.eu> wrote:
>
> In new functions print_ioctl() and print_syscall_ret_ioctl(), we don't
> check if lock_user() returns NULL and this would cause a segfault in
> thunk_print().
>
> If lock_user() returns NULL don't call thunk_print() but prints only the
> value of the (invalid) pointer.
>
> Tested with:
>
>     # cat ioctl.c
>     #include <unistd.h>
>     #include <sys/ioctl.h>
>
>     int main(void)
>     {
>         int ret;
>
>         ret = ioctl(STDOUT_FILENO, TCGETS, 0xdeadbeef);
>         ret = ioctl(STDOUT_FILENO, TCSETSF, 0xdeadbeef);
>         return 0;
>     }
>     # QEMU_STRACE= ./ioctl
>     ...
>     578 ioctl(1,TCGETS,0xdeadbeef) = -1 errno=2 (Bad address)
>     578 ioctl(1,TCSETSF,0xdeadbeef) = -1 errno=2 (Bad address)
>     ...
>     # QEMU_STRACE= passwd
>     ...
>     623 ioctl(0,TCGETS,0x3fffed04) = 0 ({})
>     623 ioctl(0,TCSETSF,{}) = 0
>     ...
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 79482e5987c8 ("linux-user: Add strace support for printing arguments of ioctl()")
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/strace.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 5235b2260cdd..39554d903911 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -889,8 +889,12 @@ print_syscall_ret_ioctl(const struct syscallname *name, abi_long ret,
>              arg_type++;
>              target_size = thunk_type_size(arg_type, 0);
>              argptr = lock_user(VERIFY_READ, arg2, target_size, 1);
> -            thunk_print(argptr, arg_type);
> -            unlock_user(argptr, arg2, target_size);
> +            if (argptr) {
> +                thunk_print(argptr, arg_type);
> +                unlock_user(argptr, arg2, target_size);
> +            } else {
> +                print_pointer(arg2, 1);
> +            }
>              qemu_log(")");
>          }
>      }
> @@ -3119,8 +3123,12 @@ print_ioctl(const struct syscallname *name,
>                      arg_type++;
>                      target_size = thunk_type_size(arg_type, 0);
>                      argptr = lock_user(VERIFY_READ, arg2, target_size, 1);
> -                    thunk_print(argptr, arg_type);
> -                    unlock_user(argptr, arg2, target_size);
> +                    if (argptr) {
> +                        thunk_print(argptr, arg_type);
> +                        unlock_user(argptr, arg2, target_size);
> +                    } else {
> +                        print_pointer(arg2, 1);
> +                    }
>                      break;
>                  }
>                  break;

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
Re: [PATCH] linux-user: Fix Coverity CID 1430271 / CID 1430272
Posted by Laurent Vivier 3 years, 9 months ago 
Le 09/07/2020 à 22:00, Laurent Vivier a écrit :
> In new functions print_ioctl() and print_syscall_ret_ioctl(), we don't
> check if lock_user() returns NULL and this would cause a segfault in
> thunk_print().
> 
> If lock_user() returns NULL don't call thunk_print() but prints only the
> value of the (invalid) pointer.
> 
> Tested with:
> 
>     # cat ioctl.c
>     #include <unistd.h>
>     #include <sys/ioctl.h>
> 
>     int main(void)
>     {
>         int ret;
> 
>         ret = ioctl(STDOUT_FILENO, TCGETS, 0xdeadbeef);
>         ret = ioctl(STDOUT_FILENO, TCSETSF, 0xdeadbeef);
>         return 0;
>     }
>     # QEMU_STRACE= ./ioctl
>     ...
>     578 ioctl(1,TCGETS,0xdeadbeef) = -1 errno=2 (Bad address)
>     578 ioctl(1,TCSETSF,0xdeadbeef) = -1 errno=2 (Bad address)
>     ...
>     # QEMU_STRACE= passwd
>     ...
>     623 ioctl(0,TCGETS,0x3fffed04) = 0 ({})
>     623 ioctl(0,TCSETSF,{}) = 0
>     ...
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 79482e5987c8 ("linux-user: Add strace support for printing arguments of ioctl()")
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/strace.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 5235b2260cdd..39554d903911 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -889,8 +889,12 @@ print_syscall_ret_ioctl(const struct syscallname *name, abi_long ret,
>              arg_type++;
>              target_size = thunk_type_size(arg_type, 0);
>              argptr = lock_user(VERIFY_READ, arg2, target_size, 1);
> -            thunk_print(argptr, arg_type);
> -            unlock_user(argptr, arg2, target_size);
> +            if (argptr) {
> +                thunk_print(argptr, arg_type);
> +                unlock_user(argptr, arg2, target_size);
> +            } else {
> +                print_pointer(arg2, 1);
> +            }
>              qemu_log(")");
>          }
>      }
> @@ -3119,8 +3123,12 @@ print_ioctl(const struct syscallname *name,
>                      arg_type++;
>                      target_size = thunk_type_size(arg_type, 0);
>                      argptr = lock_user(VERIFY_READ, arg2, target_size, 1);
> -                    thunk_print(argptr, arg_type);
> -                    unlock_user(argptr, arg2, target_size);
> +                    if (argptr) {
> +                        thunk_print(argptr, arg_type);
> +                        unlock_user(argptr, arg2, target_size);
> +                    } else {
> +                        print_pointer(arg2, 1);
> +                    }
>                      break;
>                  }
>                  break;
> 

Applied to my linux-user-for-5.1 branch.

Thanks,
Laurent

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.