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

[Laurent Vivier]

Le 09/07/2020 à 17:20, Peter Maydell a écrit :
> On Sat, 4 Jul 2020 at 17:36, Laurent Vivier <laur...@vivier.eu> wrote:
>>
>> From: Filip Bozuta <filip.boz...@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);
>> +}

Thank you Peter.

I fix that.

Laurent