From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59053) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XEMtN-0004Mf-Vl for qemu-devel@nongnu.org; Mon, 04 Aug 2014 14:21:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XEMtI-0000ob-9b for qemu-devel@nongnu.org; Mon, 04 Aug 2014 14:21:53 -0400 Message-ID: <53DFCF34.9050606@gmail.com> Date: Mon, 04 Aug 2014 13:21:40 -0500 From: Tom Musta MIME-Version: 1.0 References: <1407170739-12237-1-git-send-email-tommusta@gmail.com> <1407170739-12237-3-git-send-email-tommusta@gmail.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 02/12] linux-user: Dereference Pointer Argument to ipc/semctl Sys Call List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Riku Voipio , "qemu-ppc@nongnu.org" , QEMU Developers , Alexander Graf On 8/4/2014 12:04 PM, Peter Maydell wrote: > On 4 August 2014 17:45, Tom Musta 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 >> >> 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)