On 8/4/2014 12:04 PM, Peter Maydell wrote:
> On 4 August 2014 17:45, Tom Musta <tommu...@gmail.com> 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 <tommu...@gmail.com>
>>
>> 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)
