[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5] linux-user: syscall: ioctls: support DRM_IOCTL_VERSION
From: |
Chen Gang |
Subject: |
Re: [PATCH v5] linux-user: syscall: ioctls: support DRM_IOCTL_VERSION |
Date: |
Wed, 3 Jun 2020 19:05:18 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
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.
>> + 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.
In our case, I guess the only failure from malloc() is "no memory".
>> +}
>>
>> +static inline abi_long host_to_target_drmversion(abi_ulong target_addr,
>> + struct drm_version
>> *host_ver)
>> +{
>> + struct target_drm_version *target_ver;
>> +
>> + if (!lock_user_struct(VERIFY_WRITE, target_ver, target_addr, 0)) {
>
> I think you should not unlock_struct() in target_to_host_drmversion() so
> you don't have to lock it again here.
>
OK, thanks.
>> +static abi_long do_ioctl_drm(const IOCTLEntry *ie, uint8_t *buf_temp,
>> + int fd, int cmd, abi_long arg)
>> +{
>> + struct drm_version *ver;
>> + abi_long ret;
>> +
>> + switch (ie->host_cmd) {
>> + case DRM_IOCTL_VERSION:
>> + ver = (struct drm_version *)buf_temp;
>
> you should lock the structure here (rather than in
> target_to_host_drmversion())...
>
OK, thanks.
>> + ret = target_to_host_drmversion(ver, arg);
>> + if (is_error(ret)) {
>> + return ret;
>> + }
>> + ret = get_errno(safe_ioctl(fd, ie->host_cmd, ver));
>> + if (is_error(ret)) {
>> + unlock_drm_version(ver);
>> + return ret;
>> + }
>> + return host_to_target_drmversion(arg, ver);
>
> and unlock the structure here (rather than in host_to_target_drmversion()).
>
> You should return "ret" too.
>
OK, thanks.
>> + }
>> + return -TARGET_EFAULT;
>
> Why -TARGET_EFAULT? -TARGET_ENOSYS would be better.
>
OK, thanks.
Chen Gang.