Re: [PATCH] linux-user: fix O_NONBLOCK in signalfd4() and eventfd2() syscalls

[Laurent Vivier]

Le 13/02/2021 à 20:05, Helge Deller a écrit :
> On 2/13/21 5:38 PM, Laurent Vivier wrote:
>> Le 10/02/2021 à 07:12, Helge Deller a écrit :
>>> On the hppa target userspace binaries may call signalfd4() and
>>> eventfd2() with an old TARGET_O_NONBLOCK value of 000200004 instead of
>>> 000200000 for the "mask" syscall parameter, in which case the current
>>> emulation doesn't handle the translation to the native O_NONBLOCK value
>>> correctly.
>>>
>>> The 0x04 bit is not masked out before the new O_NONBLOCK bit is set and
>>> as such when calling the native syscall errors out with EINVAL.
>>>
>>> Fix this by introducing TARGET_O_NONBLOCK_MASK which is used to mask off
>>> all possible bits. This define defaults to TARGET_O_NONBLOCK when not
>>> defined otherwise, so for all other targets the implementation will
>>> behave as before.
>>>
>>> This patch needs to be applied on top of my previous two patches.
>>>
>>> Bug was found and patch was verified by using qemu-hppa as debian buildd
>>> server on x86_64.
>>>
>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>
>>> ---
>>>
>>> diff --git a/linux-user/hppa/target_fcntl.h b/linux-user/hppa/target_fcntl.h
>>> index 08e3a4fcb0..4eb0ec98e2 100644
>>> --- a/linux-user/hppa/target_fcntl.h
>>> +++ b/linux-user/hppa/target_fcntl.h
>>> @@ -9,6 +9,7 @@
>>>   #define HPPA_TARGET_FCNTL_H
>>>
>>>   #define TARGET_O_NONBLOCK    000200000
>>> +#define TARGET_O_NONBLOCK_MASK 000200004 /* includes old HP-UX NDELAY flag 
>>> */
>>>   #define TARGET_O_APPEND      000000010
>>>   #define TARGET_O_CREAT       000000400 /* not fcntl */
>>>   #define TARGET_O_EXCL        000002000 /* not fcntl */
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 27adee908e..3031aa342f 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -273,6 +273,11 @@ static type name (type1 arg1,type2 arg2,type3 
>>> arg3,type4 arg4,type5 arg5,    \
>>>   #define TARGET_NR__llseek TARGET_NR_llseek
>>>   #endif
>>>
>>> +/* some platforms need to mask more bits than just TARGET_O_NONBLOCK */
>>> +#ifndef TARGET_O_NONBLOCK_MASK
>>> +#define TARGET_O_NONBLOCK_MASK TARGET_O_NONBLOCK
>>> +#endif
>>> +
>>>   #define __NR_sys_gettid __NR_gettid
>>>   _syscall0(int, sys_gettid)
>>>
>>> @@ -7719,7 +7724,7 @@ 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)) {
>>> +    if (flags & ~(TARGET_O_NONBLOCK_MASK | TARGET_O_CLOEXEC)) {
>>>           return -TARGET_EINVAL;
>>>       }
>>>       if (!lock_user_struct(VERIFY_READ, target_mask, mask, 1)) {
>>> @@ -12508,7 +12513,7 @@ static abi_long do_syscall1(void *cpu_env, int num, 
>>> abi_long arg1,
>>>   #if defined(TARGET_NR_eventfd2)
>>>       case TARGET_NR_eventfd2:
>>>       {
>>> -        int host_flags = arg2 & (~(TARGET_O_NONBLOCK | TARGET_O_CLOEXEC));
>>> +        int host_flags = arg2 & (~(TARGET_O_NONBLOCK_MASK | 
>>> TARGET_O_CLOEXEC));
>>>           if (arg2 & TARGET_O_NONBLOCK) {
>>>               host_flags |= O_NONBLOCK;
>>>           }
>>>
>>
>> You might also update fcntl_flags_tbl[], the first column is the mask.
> 
> No, I intentionally did not updated the fcntl_flags_tbl[] entry.
> The check in target_to_host_bitmask() is:
>         if ((target_mask & btp->target_mask) == btp->target_bits)
> and the table entry is:
>  { TARGET_O_NONBLOCK,  TARGET_O_NONBLOCK,  O_NONBLOCK,  O_NONBLOCK,  },
> 
> Application may hand in 000200000 *or* 000200004 as value.
> So checking (value & 000200000) == 000200000 is correct for both cases.
> 

Yes, you're right.

Reviewed-by: Laurent Vivier <laurent@vivier.eu>