| Message ID | 20200506234751.7920-1-steplong@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix stack corruption when handling PR_GETDEATHSIG | expand |
Hi Stephen, On 5/7/20 1:47 AM, Stephen Long wrote: > From: Ana Pazos <apazos@quicinc.com> > > Signed-off-by: Ana Pazos <apazos@quicinc.com> > --- > Submitting this patch on behalf of Ana Pazos. The bug was triggered by > the following c file on aarch64-linux-user. This is fine, but you have to add your own S-o-b tag too. See the link from https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line By making a contribution to this project, I certify that: (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it. > >> #include <signal.h> >> #include <sys/prctl.h> >> >> int main() { >> int PDeachSig = 0; >> if (prctl(PR_GET_PDEATHSIG, &PDeachSig) == 0 && PDeachSig == SIGKILL) >> prctl(PR_SET_PDEATHSIG, 0); >> return (PDeachSig == SIGKILL); >> } > > linux-user/syscall.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 05f03919ff..4eac567f97 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -10253,10 +10253,10 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, > switch (arg1) { > case PR_GET_PDEATHSIG: > { > - int deathsig; > + uint32_t deathsig; I think the bug here is deathsig should be abi_long, the put_user_ual() call seems fine. But I don't know well user-mode, so don't rely on my comment :) > ret = get_errno(prctl(arg1, &deathsig, arg3, arg4, arg5)); > if (!is_error(ret) && arg2 > - && put_user_ual(deathsig, arg2)) { > + && put_user_u32(deathsig, arg2)) { > return -TARGET_EFAULT; > } > return ret; >
Le 07/05/2020 à 01:47, Stephen Long a écrit : > From: Ana Pazos <apazos@quicinc.com> > > Signed-off-by: Ana Pazos <apazos@quicinc.com> > --- > Submitting this patch on behalf of Ana Pazos. The bug was triggered by > the following c file on aarch64-linux-user. > >> #include <signal.h> >> #include <sys/prctl.h> >> >> int main() { >> int PDeachSig = 0; >> if (prctl(PR_GET_PDEATHSIG, &PDeachSig) == 0 && PDeachSig == SIGKILL) >> prctl(PR_SET_PDEATHSIG, 0); >> return (PDeachSig == SIGKILL); >> } Put the example in the description of the patch, it will help to understand the fix by being kept in the log. > linux-user/syscall.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 05f03919ff..4eac567f97 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -10253,10 +10253,10 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, > switch (arg1) { > case PR_GET_PDEATHSIG: > { > - int deathsig; > + uint32_t deathsig; > ret = get_errno(prctl(arg1, &deathsig, arg3, arg4, arg5)); > if (!is_error(ret) && arg2 > - && put_user_ual(deathsig, arg2)) { > + && put_user_u32(deathsig, arg2)) { > return -TARGET_EFAULT; > } > return ret; > I think you could keep the "int" and only replace put_user_ual() by put_user_s32() (to stay in line with the man page that mentions "(int *)". "int" is always a signed 32bit.. And add your "Signed-off-by". Thanks, Laurent
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 05f03919ff..4eac567f97 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -10253,10 +10253,10 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, switch (arg1) { case PR_GET_PDEATHSIG: { - int deathsig; + uint32_t deathsig; ret = get_errno(prctl(arg1, &deathsig, arg3, arg4, arg5)); if (!is_error(ret) && arg2 - && put_user_ual(deathsig, arg2)) { + && put_user_u32(deathsig, arg2)) { return -TARGET_EFAULT; } return ret;