linux-user: Drop unnecessary check in dup3 syscall

Drop the extra check in dup3() if anything other than FD_CLOEXEC (aka
O_CLOEXEC) was given. Instead simply rely on any error codes returned by
the host dup3() syscall.

Signed-off-by: Helge Deller <deller@gmx.de>

On 4/24/20 3:57 PM, Helge Deller wrote:
> Drop the extra check in dup3() if anything other than FD_CLOEXEC (aka
> O_CLOEXEC) was given. Instead simply rely on any error codes returned by
> the host dup3() syscall.
> 
> 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
> @@ -8301,12 +8310,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>   #if defined(CONFIG_DUP3) && defined(TARGET_NR_dup3)
>       case TARGET_NR_dup3:
>       {
> -        int host_flags;
> -
> -        if ((arg3 & ~TARGET_O_CLOEXEC) != 0) {
> -            return -EINVAL;
> -        }
> -        host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl);
> +        int host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl);

I don't think this is quite correct.  target_to_host_bitmask() silently 
ignores unknown bits, and a user that was relying on bit 0x40000000 to 
cause an EINVAL will not fail with this change (unless bit 0x40000000 
happens to be one of the bits translated by fcntl_flags_tbl).  The 
open() syscall is notorious for ignoring unknown bits rather than 
failing with EINVAL, and it is has come back to haunt kernel developers; 
newer syscalls like dup3() learned from the mistake, and we really do 
want to catch unsupported bits up to make it easier for future kernels 
to define meanings to those bits without them being silently swallowed 
when run on older systems that did not know what those bits meant.

On 24.04.20 23:32, Eric Blake wrote:
> On 4/24/20 3:57 PM, Helge Deller wrote:
>> Drop the extra check in dup3() if anything other than FD_CLOEXEC (aka
>> O_CLOEXEC) was given. Instead simply rely on any error codes returned by
>> the host dup3() syscall.
>>
>> 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
>> @@ -8301,12 +8310,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>>   #if defined(CONFIG_DUP3) && defined(TARGET_NR_dup3)
>>       case TARGET_NR_dup3:
>>       {
>> -        int host_flags;
>> -
>> -        if ((arg3 & ~TARGET_O_CLOEXEC) != 0) {
>> -            return -EINVAL;
>> -        }
>> -        host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl);
>> +        int host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl);
>
> I don't think this is quite correct.  target_to_host_bitmask()
> silently ignores unknown bits, and a user that was relying on bit
> 0x40000000 to cause an EINVAL will not fail with this change (unless
> bit 0x40000000 happens to be one of the bits translated by
> fcntl_flags_tbl).

True.

> The open() syscall is notorious for ignoring unknown bits rather than
> failing with EINVAL, and it is has come back to haunt kernel
> developers; newer syscalls like dup3() learned from the mistake, and
> we really do want to catch unsupported bits up to make it easier for
> future kernels to define meanings to those bits without them being
> silently swallowed when run on older systems that did not know what
> those bits meant.
Ok, I wasn't aware that it's a design goal to manually find such
cases of wrong userspace applications. But in this case, you're right
that my patch shouldn't be applied.

While looking at the code I just noticed another bug too, which needs
fixing then:
>> -        if ((arg3 & ~TARGET_O_CLOEXEC) != 0) {
>> -            return -EINVAL;
this needs to be:
>> -            return -TARGET_EINVAL;

Helge

On 4/24/20 4:47 PM, Helge Deller wrote:

>>> -        host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl);
>>> +        int host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl);
>>
>> I don't think this is quite correct.  target_to_host_bitmask()
>> silently ignores unknown bits, and a user that was relying on bit
>> 0x40000000 to cause an EINVAL will not fail with this change (unless
>> bit 0x40000000 happens to be one of the bits translated by
>> fcntl_flags_tbl).
> 
> True.
> 
>> The open() syscall is notorious for ignoring unknown bits rather than
>> failing with EINVAL, and it is has come back to haunt kernel
>> developers; newer syscalls like dup3() learned from the mistake, and
>> we really do want to catch unsupported bits up to make it easier for
>> future kernels to define meanings to those bits without them being
>> silently swallowed when run on older systems that did not know what
>> those bits meant.
> Ok, I wasn't aware that it's a design goal to manually find such
> cases of wrong userspace applications. But in this case, you're right
> that my patch shouldn't be applied.

This, and several similar ones that you also posted.

Maybe you could add a new int target_to_host_bitmask_strict(int src, 
translate_tbl, int *dst), which returns 0 when *dst is bit-for-bit 
translated from src, and returns -1 if src had bits not specified by 
translate_tbl.  In that case, the caller can then translate all usual 
bits and rely on the syscall() failure (as you tried here), but you can 
also flag -TARGET_EINVAL up front for bits not covered by the table.

> 
> While looking at the code I just noticed another bug too, which needs
> fixing then:
>>> -        if ((arg3 & ~TARGET_O_CLOEXEC) != 0) {
>>> -            return -EINVAL;
> this needs to be:
>>> -            return -TARGET_EINVAL;

Indeed.  Good catch.

On Fri, 24 Apr 2020 at 22:33, Eric Blake <eblake@redhat.com> wrote:
> I don't think this is quite correct.  target_to_host_bitmask() silently
> ignores unknown bits, and a user that was relying on bit 0x40000000 to
> cause an EINVAL will not fail with this change (unless bit 0x40000000
> happens to be one of the bits translated by fcntl_flags_tbl).  The
> open() syscall is notorious for ignoring unknown bits rather than
> failing with EINVAL, and it is has come back to haunt kernel developers;
> newer syscalls like dup3() learned from the mistake, and we really do
> want to catch unsupported bits up to make it easier for future kernels
> to define meanings to those bits without them being silently swallowed
> when run on older systems that did not know what those bits meant.

The other reason linux-user sometimes has this sort of manual
check of input values is that it can affect which errno value
is returned if a call has multiple wrong things (eg a bad
address to a pointer parameter and a bad flags value), and some
test suites care about the difference. I'm not sure that's the
case here, though. I didn't write out my reasoning back in
2017 when I made this page and don't remember it now, but my
guess is that it's just that dup3 is only supposed
to permit O_CLOEXEC, not any of the other flags that the
fcntl_flags_tbl permits and translates.

thanks
-- PMM