Re: [PATCH] linux-user: rt_sigprocmask, check read perms first

[Laurent Vivier]

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