Re: [PATCH 10/22] Implement shmid_ds conversion between host and target.

[Kariiem Taha <kariem.taha2.7@gmail.com>]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 8/19/23 02:47, Karim Taha wrote:
>> +    if (!lock_user_struct(VERIFY_WRITE, target_sd, target_addr, 0)) {
>> +        return -TARGET_EFAULT;
>> +    }
>> +    if (host_to_target_ipc_perm(target_addr, &(host_sd->shm_perm))) {
>> +        return -TARGET_EFAULT;
>> +    }
>
> While it works, ideally you wouldn't double-lock a memory range, once here and once in 
> host_to_target_ipc_perm.  You could split out the middle of the function as 
> host_to_target_ipc_perm__locked.

Hi Richard,

Can you please verify the correctness of the following refactoring?
    void host_to_target_ipc_perm__locked(abi_ulong target_addr,
            struct ipc_perm *host_ip)
    {
        struct target_ipc_perm *target_ip = g2h_untagged(target_addr);
        __put_user(host_ip->cuid, &target_ip->cuid);
        __put_user(host_ip->cgid, &target_ip->cgid);
        __put_user(host_ip->uid, &target_ip->uid);
        __put_user(host_ip->gid, &target_ip->gid);
        __put_user(host_ip->mode, &target_ip->mode);
        __put_user(host_ip->seq, &target_ip->seq);
        __put_user(host_ip->key, &target_ip->key);
    }

    abi_long host_to_target_shmid_ds(abi_ulong target_addr,
            struct shmid_ds *host_sd)
    {
        struct target_shmid_ds *target_sd;
        target_sd = lock_user(VERIFY_WRITE, target_addr, sizeof(*target_sd), 0);
        if (!target_sd){
            return -TARGET_EFAULT;
        }

        host_to_target_ipc_perm__locked(target_addr, &(host_sd->shm_perm));
        __put_user(host_sd->shm_segsz, &target_sd->shm_segsz);
        __put_user(host_sd->shm_lpid, &target_sd->shm_lpid);
        __put_user(host_sd->shm_cpid, &target_sd->shm_cpid);
        __put_user(host_sd->shm_nattch, &target_sd->shm_nattch);
        __put_user(host_sd->shm_atime, &target_sd->shm_atime);
        __put_user(host_sd->shm_dtime, &target_sd->shm_dtime);
        __put_user(host_sd->shm_ctime, &target_sd->shm_ctime);
        unlock_user_struct(target_sd, target_addr, 1);

        return 0;
    }

As far as I understood the `page_check_range` function, defined at
accel/tcg/user-exec.c::523:

-The locked range is (target_addr, target_addr + sizeof(target_ipc_perm) -1) in case of
     host_to_target_ipc_perm function.

-The locked range is (target_addr, taregt_addr + sizeof(target_shmid_ds) -1) in case of
      host_to_target_shmid_ds function.

Since `host_to_target_shmid_ds` struct has larger size, in the original
code, is the sucess of the first lock guarantees the sucess of the
second?

If I got it wrong, please elaborate further.

If I'm correct, do you think I should call g2h_untagged in
`host_to_target_ipc_perm__locked` directly, or should I receive it as a
paremeter?

--
Kariiem Taha