diff mbox series

linux-user: Drop unnecessary check in signalfd4 syscall

Message ID 20200424210422.GB26282@ls3530.fritz.box
State New
Headers show
Series linux-user: Drop unnecessary check in signalfd4 syscall | expand

Commit Message

Helge Deller April 24, 2020, 9:04 p.m. UTC 
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 <deller@gmx.de>

Comments

Laurent Vivier April 25, 2020, 8:39 a.m. UTC | #1 
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 <deller@gmx.de>
> 
> 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.

We can remove the flags checking but we should also change TARGET_EFAULT
by TARGET_EINVAL.

Thanks,
Laurent
Helge Deller April 25, 2020, 9:24 a.m. UTC | #2 
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 <deller@gmx.de>
>>
>> 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.

Helge
Laurent Vivier April 25, 2020, 10:03 a.m. UTC | #3 
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 <deller@gmx.de>
>>>
>>> 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
Helge Deller April 25, 2020, 9:48 p.m. UTC | #4 
On 25.04.20 12:03, Laurent Vivier wrote:
> 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 <deller@gmx.de>
>>>>
>>>> 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.

The manpage doesn't mention any error code for a bad mask address, and
IMHO the manpage should reflect the real coding, not vice versa.
I've just sent a RFC patch to the kernel mailing list.
It's not a critical problem, but it would be nice to be at least consistent
across the implementations. Either both should return always EINVAL,
or both should return EFAULT. If it gets accepted I'll send a patch
to correct the manpage accordingly afterwards, and then we can adjust
qemu.

Helge
diff mbox series

Patch

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;
     }