Re: [PATCH] linux-user: Fix 'semop()' and 'semtimedop()' implementation

[Laurent Vivier]

Le 12/08/2020 à 16:07, Filip Bozuta a écrit :
> The implementations of syscalls 'semop()' and 'semtimedop()' in
> file 'syscall.c' use function 'target_to_host_sembuf()' to convert
> values of 'struct sembuf' from host to target. However, before this
> conversion it should be check whether the number of semaphore operations
> 'nsops' is not bigger than maximum allowed semaphor operations per
> syscall: 'SEMOPM'. In these cases, errno 'E2BIG' ("Arg list too long")
> should be set. But the implementation will set errno 'EFAULT' ("Bad address")
> in this case since the conversion from target to host fails.
> 
> This was confirmed with the LTP test for 'semop()' ('ipc/semop/semop02') in
> test case where 'nsops' is greater than SEMOPM with unaproppriate errno 
> EFAULT:
> 
> semop02.c:130: FAIL: semop failed unexpectedly; expected: E2BIG: EFAULT (14)
> 
> This patch changes this by adding a check whether 'nsops' is bigger than
> 'SEMOPM' before the conversion function 'target_to_host_sembuf()' is called.
> After the changes from this patch, the test works fine along with the other
> LTP testcases for 'semop()'):
> 
> semop02.c:126: PASS: semop failed as expected: E2BIG (7)
> 
> Implementation notes:
> 
>     A target value ('TARGET_SEMOPM') was added for 'SEMOPM' as to be sure
>     in case the value is not available for some targets.
> 
> Signed-off-by: Filip Bozuta <Filip.Bozuta@syrmia.com>
> ---
>  linux-user/syscall.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1211e759c2..4743a5bef2 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3899,6 +3899,8 @@ static inline abi_long target_to_host_sembuf(struct 
> sembuf *host_sembuf,
>    (__nsops), 0, (__sops), (__timeout)
>  #endif
>  
> +#define TARGET_SEMOPM 500
> +

I think you could use directly SEMOPM as it is the same everywhere.

>  static inline abi_long do_semtimedop(int semid,
>                                       abi_long ptr,
>                                       unsigned nsops,
> @@ -3915,8 +3917,13 @@ static inline abi_long do_semtimedop(int semid,
>          }
>      }
>  
> -    if (target_to_host_sembuf(sops, ptr, nsops))
> +    if (nsops > TARGET_SEMOPM) {

You might move the check before allocation of the memory for sops.

> +        return -TARGET_E2BIG;
> +    }
> +
> +    if (target_to_host_sembuf(sops, ptr, nsops)) {
>          return -TARGET_EFAULT;
> +    }
>  
>      ret = -TARGET_ENOSYS;
>  #ifdef __NR_semtimedop
> 

Thanks,
Laurent