| Message ID | 20220106220038.3658586-1-venture@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: rt_sigprocmask, check read perms first | expand |
Le 06/01/2022 à 23:00, Patrick Venture a écrit : > From: Shu-Chun Weng <scw@google.com> > > Linux kernel does it this way (checks read permission before validating `how`) > and the latest version of ABSL's `AddressIsReadable()` depends on this > behavior. > > c.f. https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147 > Reviewed-by: Patrick Venture <venture@google.com> > Signed-off-by: Shu-Chun Weng <scw@google.com> > --- > linux-user/syscall.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index ce9d64896c..3070d31f34 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -9491,6 +9491,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, > } > > if (arg2) { > + if (!(p = lock_user(VERIFY_READ, arg2, sizeof(target_sigset_t), 1))) > + return -TARGET_EFAULT; > + target_to_host_sigset(&set, p); > + unlock_user(p, arg2, 0); > + set_ptr = &set; > switch(how) { > case TARGET_SIG_BLOCK: > how = SIG_BLOCK; > @@ -9504,11 +9509,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, > default: > return -TARGET_EINVAL; > } > - if (!(p = lock_user(VERIFY_READ, arg2, sizeof(target_sigset_t), 1))) > - return -TARGET_EFAULT; > - target_to_host_sigset(&set, p); > - unlock_user(p, arg2, 0); > - set_ptr = &set; > } else { > how = 0; > set_ptr = NULL; I know it's only code move but generally we also update the style to pass scripts/checkpatch.pl successfully. Could you also update TARGET_NR_sigprocmask in the same way as it seems the kernel behaves like this too in this case? Thanks, Laurent
On Sat, Jan 8, 2022 at 10:16 AM Laurent Vivier <laurent@vivier.eu> wrote: > Le 06/01/2022 à 23:00, Patrick Venture a écrit : > > From: Shu-Chun Weng <scw@google.com> > > > > Linux kernel does it this way (checks read permission before validating > `how`) > > and the latest version of ABSL's `AddressIsReadable()` depends on this > > behavior. > > > > c.f. > https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147 > > Reviewed-by: Patrick Venture <venture@google.com> > > Signed-off-by: Shu-Chun Weng <scw@google.com> > > --- > > linux-user/syscall.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > index ce9d64896c..3070d31f34 100644 > > --- a/linux-user/syscall.c > > +++ b/linux-user/syscall.c > > @@ -9491,6 +9491,11 @@ static abi_long do_syscall1(void *cpu_env, int > num, abi_long arg1, > > } > > > > if (arg2) { > > + if (!(p = lock_user(VERIFY_READ, arg2, > sizeof(target_sigset_t), 1))) > > + return -TARGET_EFAULT; > > + target_to_host_sigset(&set, p); > > + unlock_user(p, arg2, 0); > > + set_ptr = &set; > > switch(how) { > > case TARGET_SIG_BLOCK: > > how = SIG_BLOCK; > > @@ -9504,11 +9509,6 @@ static abi_long do_syscall1(void *cpu_env, int > num, abi_long arg1, > > default: > > return -TARGET_EINVAL; > > } > > - if (!(p = lock_user(VERIFY_READ, arg2, > sizeof(target_sigset_t), 1))) > > - return -TARGET_EFAULT; > > - target_to_host_sigset(&set, p); > > - unlock_user(p, arg2, 0); > > - set_ptr = &set; > > } else { > > how = 0; > > set_ptr = NULL; > > I know it's only code move but generally we also update the style to pass > scripts/checkpatch.pl > successfully. > That is a reasonable request, however, can I just send a follow-on patch? I didn't write this one and I honestly don't know much about it, but I don't mind doing the cleanup > > Could you also update TARGET_NR_sigprocmask in the same way as it seems > the kernel behaves like this > too in this case? > I can take a look. I would prefer then to also prefetch the style fixup in a preceding patch. I don't recall seeing whether qemu supports clang-format. > > Thanks, > Laurent > Patrick
Hi Patrick, Le 11/01/2022 à 21:14, Patrick Venture a écrit : > > > On Sat, Jan 8, 2022 at 10:16 AM Laurent Vivier <laurent@vivier.eu <mailto:laurent@vivier.eu>> wrote: > > Le 06/01/2022 à 23:00, Patrick Venture a écrit : > > From: Shu-Chun Weng <scw@google.com <mailto:scw@google.com>> > > > > Linux kernel does it this way (checks read permission before validating `how`) > > and the latest version of ABSL's `AddressIsReadable()` depends on this > > behavior. > > > > c.f. > https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147 > <https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147> > > Reviewed-by: Patrick Venture <venture@google.com <mailto:venture@google.com>> > > Signed-off-by: Shu-Chun Weng <scw@google.com <mailto:scw@google.com>> > > --- > > linux-user/syscall.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > index ce9d64896c..3070d31f34 100644 > > --- a/linux-user/syscall.c > > +++ b/linux-user/syscall.c > > @@ -9491,6 +9491,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, > > } > > > > if (arg2) { > > + if (!(p = lock_user(VERIFY_READ, arg2, sizeof(target_sigset_t), 1))) > > + return -TARGET_EFAULT; > > + target_to_host_sigset(&set, p); > > + unlock_user(p, arg2, 0); > > + set_ptr = &set; > > switch(how) { > > case TARGET_SIG_BLOCK: > > how = SIG_BLOCK; > > @@ -9504,11 +9509,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, > > default: > > return -TARGET_EINVAL; > > } > > - if (!(p = lock_user(VERIFY_READ, arg2, sizeof(target_sigset_t), 1))) > > - return -TARGET_EFAULT; > > - target_to_host_sigset(&set, p); > > - unlock_user(p, arg2, 0); > > - set_ptr = &set; > > } else { > > how = 0; > > set_ptr = NULL; > > I know it's only code move but generally we also update the style to pass scripts/checkpatch.pl > <http://checkpatch.pl> > successfully. > > > That is a reasonable request, however, can I just send a follow-on patch? I didn't write this one > and I honestly don't know much about it, but I don't mind doing the cleanup > > > Could you also update TARGET_NR_sigprocmask in the same way as it seems the kernel behaves like > this > too in this case? > > > I can take a look. I would prefer then to also prefetch the style fixup in a preceding patch. I > don't recall seeing whether qemu supports clang-format. > There is no problem. You can keep this patch unmodified, and add patches to fix the problems. I only ask to have all the patches in one series. Thanks, Laurent
On Tue, Jan 11, 2022 at 12:50 PM Laurent Vivier <laurent@vivier.eu> wrote: > Hi Patrick, > > Le 11/01/2022 à 21:14, Patrick Venture a écrit : > > > > > > On Sat, Jan 8, 2022 at 10:16 AM Laurent Vivier <laurent@vivier.eu > <mailto:laurent@vivier.eu>> wrote: > > > > Le 06/01/2022 à 23:00, Patrick Venture a écrit : > > > From: Shu-Chun Weng <scw@google.com <mailto:scw@google.com>> > > > > > > Linux kernel does it this way (checks read permission before > validating `how`) > > > and the latest version of ABSL's `AddressIsReadable()` depends on > this > > > behavior. > > > > > > c.f. > > > https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147 > > < > https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147 > > > > > Reviewed-by: Patrick Venture <venture@google.com <mailto: > venture@google.com>> > > > Signed-off-by: Shu-Chun Weng <scw@google.com <mailto: > scw@google.com>> > > > --- > > > linux-user/syscall.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > > index ce9d64896c..3070d31f34 100644 > > > --- a/linux-user/syscall.c > > > +++ b/linux-user/syscall.c > > > @@ -9491,6 +9491,11 @@ static abi_long do_syscall1(void *cpu_env, > int num, abi_long arg1, > > > } > > > > > > if (arg2) { > > > + if (!(p = lock_user(VERIFY_READ, arg2, > sizeof(target_sigset_t), 1))) > > > + return -TARGET_EFAULT; > > > + target_to_host_sigset(&set, p); > > > + unlock_user(p, arg2, 0); > > > + set_ptr = &set; > > > switch(how) { > > > case TARGET_SIG_BLOCK: > > > how = SIG_BLOCK; > > > @@ -9504,11 +9509,6 @@ static abi_long do_syscall1(void *cpu_env, > int num, abi_long arg1, > > > default: > > > return -TARGET_EINVAL; > > > } > > > - if (!(p = lock_user(VERIFY_READ, arg2, > sizeof(target_sigset_t), 1))) > > > - return -TARGET_EFAULT; > > > - target_to_host_sigset(&set, p); > > > - unlock_user(p, arg2, 0); > > > - set_ptr = &set; > > > } else { > > > how = 0; > > > set_ptr = NULL; > > > > I know it's only code move but generally we also update the style to > pass scripts/checkpatch.pl > > <http://checkpatch.pl> > > successfully. > > > > > > That is a reasonable request, however, can I just send a follow-on > patch? I didn't write this one > > and I honestly don't know much about it, but I don't mind doing the > cleanup > > > > > > Could you also update TARGET_NR_sigprocmask in the same way as it > seems the kernel behaves like > > this > > too in this case? > > > > > > I can take a look. I would prefer then to also prefetch the style fixup > in a preceding patch. I > > don't recall seeing whether qemu supports clang-format. > > > > There is no problem. You can keep this patch unmodified, and add patches > to fix the problems. > > I only ask to have all the patches in one series. > Will take a swing at this for v2. > > Thanks, > Laurent > >
On Tue, Jan 11, 2022 at 3:06 PM Patrick Venture <venture@google.com> wrote: > > > On Tue, Jan 11, 2022 at 12:50 PM Laurent Vivier <laurent@vivier.eu> wrote: > >> Hi Patrick, >> >> Le 11/01/2022 à 21:14, Patrick Venture a écrit : >> > >> > >> > On Sat, Jan 8, 2022 at 10:16 AM Laurent Vivier <laurent@vivier.eu >> <mailto:laurent@vivier.eu>> wrote: >> > >> > Le 06/01/2022 à 23:00, Patrick Venture a écrit : >> > > From: Shu-Chun Weng <scw@google.com <mailto:scw@google.com>> >> > > >> > > Linux kernel does it this way (checks read permission before >> validating `how`) >> > > and the latest version of ABSL's `AddressIsReadable()` depends >> on this >> > > behavior. >> > > >> > > c.f. >> > >> https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147 >> > < >> https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147 >> > >> > > Reviewed-by: Patrick Venture <venture@google.com <mailto: >> venture@google.com>> >> > > Signed-off-by: Shu-Chun Weng <scw@google.com <mailto: >> scw@google.com>> >> > > --- >> > > linux-user/syscall.c | 10 +++++----- >> > > 1 file changed, 5 insertions(+), 5 deletions(-) >> > > >> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> > > index ce9d64896c..3070d31f34 100644 >> > > --- a/linux-user/syscall.c >> > > +++ b/linux-user/syscall.c >> > > @@ -9491,6 +9491,11 @@ static abi_long do_syscall1(void >> *cpu_env, int num, abi_long arg1, >> > > } >> > > >> > > if (arg2) { >> > > + if (!(p = lock_user(VERIFY_READ, arg2, >> sizeof(target_sigset_t), 1))) >> > > + return -TARGET_EFAULT; >> > > + target_to_host_sigset(&set, p); >> > > + unlock_user(p, arg2, 0); >> > > + set_ptr = &set; >> > > switch(how) { >> > > case TARGET_SIG_BLOCK: >> > > how = SIG_BLOCK; >> > > @@ -9504,11 +9509,6 @@ static abi_long do_syscall1(void >> *cpu_env, int num, abi_long arg1, >> > > default: >> > > return -TARGET_EINVAL; >> > > } >> > > - if (!(p = lock_user(VERIFY_READ, arg2, >> sizeof(target_sigset_t), 1))) >> > > - return -TARGET_EFAULT; >> > > - target_to_host_sigset(&set, p); >> > > - unlock_user(p, arg2, 0); >> > > - set_ptr = &set; >> > > } else { >> > > how = 0; >> > > set_ptr = NULL; >> > >> > I know it's only code move but generally we also update the style >> to pass scripts/checkpatch.pl >> > <http://checkpatch.pl> >> > successfully. >> > >> > >> > That is a reasonable request, however, can I just send a follow-on >> patch? I didn't write this one >> > and I honestly don't know much about it, but I don't mind doing the >> cleanup >> > >> > >> > Could you also update TARGET_NR_sigprocmask in the same way as it >> seems the kernel behaves like >> > this >> > too in this case? >> > >> > >> > I can take a look. I would prefer then to also prefetch the style >> fixup in a preceding patch. I >> > don't recall seeing whether qemu supports clang-format. >> > >> >> There is no problem. You can keep this patch unmodified, and add patches >> to fix the problems. >> >> I only ask to have all the patches in one series. >> > > Will take a swing at this for v2. > Laurent, I spent some time today going over the patches and digging into what they're actually doing and I think I can make this two patches, both with the style changes squashed and will send in a v2 today. Thanks > > >> >> Thanks, >> Laurent >> >>
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index ce9d64896c..3070d31f34 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -9491,6 +9491,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, } if (arg2) { + if (!(p = lock_user(VERIFY_READ, arg2, sizeof(target_sigset_t), 1))) + return -TARGET_EFAULT; + target_to_host_sigset(&set, p); + unlock_user(p, arg2, 0); + set_ptr = &set; switch(how) { case TARGET_SIG_BLOCK: how = SIG_BLOCK; @@ -9504,11 +9509,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, default: return -TARGET_EINVAL; } - if (!(p = lock_user(VERIFY_READ, arg2, sizeof(target_sigset_t), 1))) - return -TARGET_EFAULT; - target_to_host_sigset(&set, p); - unlock_user(p, arg2, 0); - set_ptr = &set; } else { how = 0; set_ptr = NULL;