| Message ID | 1407170739-12237-3-git-send-email-tommusta@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 4 August 2014 17:45, Tom Musta <tommusta@gmail.com> 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 <tommusta@gmail.com> > > 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
On 8/4/2014 12:04 PM, Peter Maydell wrote: > On 4 August 2014 17:45, Tom Musta <tommusta@gmail.com> 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 <tommusta@gmail.com> >> >> 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)
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); break; + } case IPCOP_msgget: ret = get_errno(msgget(first, second));
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 <tommusta@gmail.com>