Re: [PATCH v5] linux-user: syscall: ioctls: support DRM_IOCTL_VERSION

[Laurent Vivier <laurent@vivier.eu>]

Le 03/06/2020 à 13:05, Chen Gang a écrit :
> On 2020/6/3 下午5:49, Laurent Vivier wrote:
>> Le 03/06/2020 à 03:08, chengang@emindsoft.com.cn a écrit :
>>> +#ifdef HAVE_DRM_H
>>> +
>>> +static void unlock_drm_version(struct drm_version *host_ver)
>>> +{
>>> +    if (host_ver->name) {
>>> +        unlock_user(host_ver->name, 0UL, 0);
>>
>> unlock_user() allows to have a NULL host pointer parameter, so you don't
>> need to check. But you must provide the target pointer, with the length.
>> The same below.
>>
> 
> As far as I know, the unlock_user is defined in
> include/exec/softmmu-semi.h, which only checks the len before calling
> cpu_memory_rw_debug, and only calls free() for the host pointer.
> 
> So we have to be sure that the host pointer must be valid. When we pass
> 0 length to unlock_user, we want it to free host pointer only.

No, it is defined in our case in linux-user/qemu.h, and associated
comment is:

/* Unlock an area of guest memory.  The first LEN bytes must be
   flushed back to guest memory. host_ptr = NULL is explicitly
   allowed and does nothing. */

> 
>>> +    if (host_ver->desc_len) {
>>> +        host_ver->desc = lock_user(VERIFY_WRITE, target_ver->desc,
>>> +                                   target_ver->desc_len, 0);
>>> +        if (!host_ver->desc) {
>>> +            goto err;
>>> +        }
>>> +    }
>>> +
>>> +    unlock_user_struct(target_ver, target_addr, 0);
>>> +    return 0;
>>> +err:
>>> +    unlock_drm_version(host_ver);
>>> +    unlock_user_struct(target_ver, target_addr, 0);
>>> +    return -ENOMEM;
>>
>> In fact it should be -TARGET_EFAULT: it has failed because of access rights.
>>
> 
> As far as I know, the lock_user is defined in
> include/exec/softmmu-semi.h. If the parameter 'copy' is 0 (in our case),
> lock_user will only malloc a host pointer and return it.

No, in linux-user/qemu.h:

/* Lock an area of guest memory into the host.  If copy is true then the
   host area will have the same contents as the guest.  */

> In our case, I guess the only failure from malloc() is "no memory".

See use-cases in syscall.c, they all fail with -TARGET_EFAULT.

Thanks,
Laurent