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

[Helge Deller]

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 <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
>> @@ -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