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
