| Message ID | 20200709200010.1016741-1-laurent@vivier.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: Fix Coverity CID 1430271 / CID 1430272 | expand |
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
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
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;
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(-)