Re: [PULL 11/12] linux-user: Add strace support for printing arguments of ioctl()

[Peter Maydell]

On Sat, 4 Jul 2020 at 17:36, Laurent Vivier <laurent@vivier.eu> wrote:
>
> From: Filip Bozuta <Filip.Bozuta@syrmia.com>
>
> This patch implements functionality for strace argument printing for ioctls.

Hi; Coverity points out some issues in this change:


> +#ifdef TARGET_NR_ioctl
> +static void
> +print_syscall_ret_ioctl(const struct syscallname *name, abi_long ret,
> +                        abi_long arg0, abi_long arg1, abi_long arg2,
> +                        abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +    print_syscall_err(ret);
> +
> +    if (ret >= 0) {
> +        qemu_log(TARGET_ABI_FMT_ld, ret);
> +
> +        const IOCTLEntry *ie;
> +        const argtype *arg_type;
> +        void *argptr;
> +        int target_size;
> +
> +        for (ie = ioctl_entries; ie->target_cmd != 0; ie++) {
> +            if (ie->target_cmd == arg1) {
> +                break;
> +            }
> +        }
> +
> +        if (ie->target_cmd == arg1 &&
> +           (ie->access == IOC_R || ie->access == IOC_RW)) {
> +            arg_type = ie->arg_type;
> +            qemu_log(" (");
> +            arg_type++;
> +            target_size = thunk_type_size(arg_type, 0);
> +            argptr = lock_user(VERIFY_READ, arg2, target_size, 1);

Here we fail to check that lock_user() didn't return NULL...

> +            thunk_print(argptr, arg_type);

...which would cause a segfault in thunk_print().
This is CID 1430271.

> +            unlock_user(argptr, arg2, target_size);
> +            qemu_log(")");
> +        }
> +    }
> +    qemu_log("\n");
> +}
> +#endif

> +#ifdef TARGET_NR_ioctl
> +static void
> +print_ioctl(const struct syscallname *name,
> +            abi_long arg0, abi_long arg1, abi_long arg2,
> +            abi_long arg3, abi_long arg4, abi_long arg5)
> +{

> +            case TYPE_PTR:
> +                switch (ie->access) {
> +                case IOC_R:
> +                    print_pointer(arg2, 1);
> +                    break;
> +                case IOC_W:
> +                case IOC_RW:
> +                    arg_type++;
> +                    target_size = thunk_type_size(arg_type, 0);
> +                    argptr = lock_user(VERIFY_READ, arg2, target_size, 1);
> +                    thunk_print(argptr, arg_type);

Similarly here we need to check that lock_user didn't fail.
This is CID 1430272.

> +                    unlock_user(argptr, arg2, target_size);
> +                    break;
> +                }
> +                break;
> +            default:
> +                g_assert_not_reached();
> +            }
> +        }
> +    }
> +    print_syscall_epilogue(name);
> +}

thanks
-- PMM