Re: [Qemu-devel] [PATCH] linux-user: fix ipc(SEMCTL, ...) argument handling

[Peter Maydell]

On 23 June 2014 23:18, Paul Burton <address@hidden> wrote:
> On Mon, Jun 23, 2014 at 11:12:42PM +0100, Peter Maydell wrote:
>> On 23 June 2014 22:40, Paul Burton <address@hidden> wrote:
>> > The ptr argument to the ipc syscall was incorrectly being used as the
>> > value of the argument union for the SEMCTL call. It is actually, as its
>> > name would suggest, a pointer to that union.
>>
>> Have you checked this on other architectures than MIPS?
>> I have a vague recollection that there are between-arch
>> differences regarding handling of the semctl argument...
>
> I haven't tried running code for any other targets, but the pointer is
> dereferenced from generic code in Linux, see ipc/syscall.c:
>
>   
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/ipc/syscall.c#n39

I see that code has a NULL pointer check which your patch doesn't...

>> Also, VERIFY_READ doesn't seem right for some of the
>> semctl operations which will modify the target_semun.

> That part I think you're right about, I'll switch to VERIFY_WRITE.

On the other hand that doesn't line up with the kernel code,
which just does a get_user() of a single target_ulong and
passes that to the sys_semctl function (which then might
interpret it as a user pointer to a thing that needs locking
and might be written to). That would suggest that you
should be using get_user_ual() here, passing an abi_ulong
into do_semctl() and probably fixing up that function and its
callers.

Basically I think the semctl code is probably buggy in a
number of ways and so I'm dubious about a patch that's
trying to make a very small change to it without looking
at the larger picture and testing and fixing bugs on more
than one architecture.

(http://patchwork.ozlabs.org/patch/217249/ is a previous
attempt at fixing up semctl; on reflection I may have been
wrong about the relevance of compat_sys_semctl, though.)

thanks
-- PMM