Re: [Qemu-devel] [PATCH 02/12] linux-user: Dereference Pointer Argument to ipc/semctl Sys Call

[Tom Musta]

On 8/4/2014 12:04 PM, Peter Maydell wrote:
> On 4 August 2014 17:45, Tom Musta <address@hidden> wrote:
>> When the ipc system call is used to wrap a semctl system call,
>> the ptr argument to ipc needs to be dereferenced prior to passing
>> it to the semctl handler.  This is because the fourth argument to
>> semctl is a union and not a pointer to a union.
>>
>> Signed-off-by: Tom Musta <address@hidden>
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 540001c..229c482 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -3135,9 +3135,15 @@ static abi_long do_ipc(unsigned int call, int first,
>>          ret = get_errno(semget(first, second, third));
>>          break;
>>
>> -    case IPCOP_semctl:
>> -        ret = do_semctl(first, second, third, (union 
>> target_semun)(abi_ulong) ptr);
>> +    case IPCOP_semctl: {
>> +        /* The semun argument to semctl is passed by value, so dereference 
>> the
>> +         * ptr argument. */
>> +        abi_ulong atptr;
>> +        get_user_ual(atptr, (abi_ulong)ptr);
>> +        ret = do_semctl(first, second, third,
>> +                (union target_semun)(abi_ulong) atptr);
> 
> My review comments on this patch from Paul Burton:
> http://patchwork.ozlabs.org/patch/363201/
> apply here too: the change here to use get_user_ual()
> looks plausible, except that do_semctl() writes to the
> target_su in some cases, so how is this supposed to
> pass the value back to the caller? Probably do_semctl()
> is buggy, but the whole thing needs to be scrutinized
> and fixed, not just this little corner...
> 
> thanks
> -- PMM
> 

Thanks for your review of these patches, Peter.

It appears that Paul never resolved your concerns and resubmitted his patch (?).
To be honest, I'm not sure yet that I yet see what has you concerned, but I
will attempt an end-to-end review of the semctl path. (QEMU, glibc, kernel)