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

[Paul Burton]

On Mon, Jun 23, 2014 at 11:35:17PM +0100, Peter Maydell wrote:
> >> 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...

True, a NULL pointer would lead to EFAULT with my patch where the kernel
will give EINVAL. I'll fix it.
 
> >> 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).

We've crossed emails, I just noted the same thing :)

> 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.

Well as far as I can tell the union will always be the size of a long
anyway, so I see no harm in using lock_user(_struct) as I did. I'll
switch if you insist, but the result will be the same.

> Basically I think the semctl code is probably buggy in a
> number of ways

Perhaps, I suspect you know better than I.

> and so I'm dubious about a patch that's
> trying to make a very small change to it

Isn't that precisely how good bisectable bug fixes should be made?

> without looking
> at the larger picture and testing and fixing bugs on more
> than one architecture.

I pointed you to the kernel code which dereferences the pointer & it's
quite clearly architecture neutral, so I'm not sure what you're getting
at with the architecture comment.

There's quite clearly a bug in QEMU here, and this patch fixes it. I
hope you're not saying you don't want it merged because it doesn't fix
some other hypothetical bugs in the same area.

> (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.)

In terms of the compat_ wrappers, note that compat_sys_ipc also
dereferences the argument as a pointer:

  
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/ipc/compat.c#n350

And that compat_sys_semctl does not. As far as I can see they both match
the behaviour of the non-compat versions with regards to this, so that
seems irrelevant.

I do agree that the patch you link to is wrong though, I'm guessing the
author had confused semctl(...) and ipc(SEMCTL, ...).

Thanks,
    Paul

signature.asc
Description: Digital signature