Re: [PATCH] linux-user: Drop unnecessary check in signalfd4 syscall

[Laurent Vivier]

Le 25/04/2020 à 11:24, Helge Deller a écrit :
> On 25.04.20 10:39, Laurent Vivier wrote:
>> Le 24/04/2020 à 23:04, Helge Deller a écrit :
>>> The signalfd4() syscall takes optional O_NONBLOCK and O_CLOEXEC fcntl
>>> flags.  If the user gave any other invalid flags, the host syscall will
>>> return correct error codes, so simply drop the extra check here.
>>>
>>> Signed-off-by: Helge Deller <address@hidden>
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 05f03919ff..ebf0d38321 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -7176,9 +7176,6 @@ static abi_long do_signalfd4(int fd, abi_long mask, 
>>> int flags)
>>>      sigset_t host_mask;
>>>      abi_long ret;
>>>
>>> -    if (flags & ~(TARGET_O_NONBLOCK | TARGET_O_CLOEXEC)) {
>>> -        return -TARGET_EINVAL;
>>> -    }
>>>      if (!lock_user_struct(VERIFY_READ, target_mask, mask, 1)) {
>>>          return -TARGET_EFAULT;
>>>      }
>>>
>>
>> Perhaps we want to trigger the TARGET_EINVAL before the TARGET_EFAULT if
>> we have both cases?
>>
>> But I've checked the kernel, and the kernel does a copy_from_user()
>> before checking the flags, but it returns EINVAL rather than EFAULT.
> 
> That's not the full picture, since the kernel is not consistent here!
> In the compat-case (32bit userspace on 64bit kernel) it returns correctly
> EINVAL and EFAULT:
>         if (sigsetsize != sizeof(compat_sigset_t))
>                 return -EINVAL;
>         if (get_compat_sigset(&mask, user_mask))
>                 return -EFAULT;
> while in the non-compat case it returns EINVAL only:
>         if (sizemask != sizeof(sigset_t) ||
>             copy_from_user(&mask, user_mask, sizeof(mask)))
>                 return -EINVAL;
> 
> I think the kernel should be fixed here...
> 
>> We can remove the flags checking but we should also change TARGET_EFAULT
>> by TARGET_EINVAL.
> 
> According to the different behaviour of the kernel mentioned above
> you won't get it correct either way.

If we refer to manpage, EFAULT is not one of possible errors.

Thanks,
Laurent