From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50368) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzDJV-0006iF-CF for qemu-devel@nongnu.org; Mon, 23 Jun 2014 19:06:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WzDJP-0001Nx-Lt for qemu-devel@nongnu.org; Mon, 23 Jun 2014 19:06:13 -0400 Received: from mail-wg0-x234.google.com ([2a00:1450:400c:c00::234]:54802) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzDJP-0001Nj-Az for qemu-devel@nongnu.org; Mon, 23 Jun 2014 19:06:07 -0400 Received: by mail-wg0-f52.google.com with SMTP id b13so7045503wgh.23 for ; Mon, 23 Jun 2014 16:06:06 -0700 (PDT) Sender: Paul Burton Date: Tue, 24 Jun 2014 00:06:02 +0100 From: Paul Burton Message-ID: <20140623230602.GE4377@gmail.com> References: <1403559614-4096-1-git-send-email-paul@archlinuxmips.org> <20140623221825.GC4377@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Izn7cH1Com+I3R9J" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH] linux-user: fix ipc(SEMCTL, ...) argument handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Riku Voipio , QEMU Developers , Paul Burton --Izn7cH1Com+I3R9J Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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/i= pc/syscall.c#n39 >=20 > 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. =20 > >> Also, VERIFY_READ doesn't seem right for some of the > >> semctl operations which will modify the target_semun. >=20 > > That part I think you're right about, I'll switch to VERIFY_WRITE. >=20 > 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/c= ompat.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 --Izn7cH1Com+I3R9J Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJTqLLaAAoJENzvn0paErs5dZ4P/RyC5n45iZxdgI9854h3EJ/c 9fqwc0qklgDuYHhKAs9spm7iDVziIyo+DvYlH/zvNL4ZS2LkWVbASxMkugXLUq6Q 6g1+lt36ncCkR1kSihZcONbSXtK2hAJpEAUSm+IXvF8JZDXGV21LCuom0OVOWJVS UDtdXF7GS/oWYx31qBd9Zdwlxtc0Wv/xNn1kAFZR9OBq8NJKL80pXX5djUBEgc8I Gd9PMRbnz2fggrKWsf6rHcNxISLLlRsO9Q8MSY1T/nOaBrkihFWVLa5sHYcnf0YA rLlN9G33jy2UGhs/e+5jn6FcavXks6vjIqL71NIj1O3F9/SXeeX37ShW9PeTYuvA g3nsDH5OD9QgVT2slamMwvptoCmN/ngccaFxwNQAFqUoYWy/wfO1iKUmVXfe2bny wT2Z8lWesCsEPi4wkFef7MTW53b5WwORfWYbR7uwVPUfc5RR+ctNqsRSDIeHb+VH JIJ4bW6oFKo73lf9yvcNpejV3gUEmg4oYuiQGAoUDo4Wl56y6WReYM+WACjgj5w7 niLm7IuSy81tNlQ03mr4aBgVeadjFUdyGu8qsoUaYZ2XbySJ4SKltN1meViF93IM JYOyW/MxziyL86tSSMkqAOyV6zL1O09cQBKxn8qSfYdJb8B0c8SK8+/oankiIc0U 3da1sfzE+noYb/LYblFs =oe3v -----END PGP SIGNATURE----- --Izn7cH1Com+I3R9J--