linux-user: Fix 'clock_nanosleep()' implementation

[PATCH] linux-user: Fix 'clock_nanosleep()' implementation

Posted by Filip Bozuta 3 years, 9 months ago

Implementation of syscall 'clock_nanosleep()' in 'syscall.c' uses
functions 'target_to_host_timespec()' and 'host_to_target_timespec()'
to transfer the value of 'struct timespec' between target and host.
However, the implementation doesn't check whether this conversion
succeeds and thus can return an unaproppriate error instead of 'EFAULT'
that is expected. This was confirmed with the modified LTP test suite
where testcases with bad 'struct timespec' adress for 'clock_nanosleep()'
were added. This modified LTP suite can be found at:
https://github.com/bozutaf/ltp

(Patch with this new test case will be sent to LTP mailing list soon)

Signed-off-by: Filip Bozuta <Filip.Bozuta@syrmia.com>
---
 linux-user/syscall.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f5c4f6b95d..9f06dde947 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11828,7 +11828,9 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
     case TARGET_NR_clock_nanosleep:
     {
         struct timespec ts;
-        target_to_host_timespec(&ts, arg3);
+        if (target_to_host_timespec(&ts, arg3)) {
+            return -TARGET_EFAULT;
+        }
         ret = get_errno(safe_clock_nanosleep(arg1, arg2,
                                              &ts, arg4 ? &ts : NULL));
         /*
@@ -11836,8 +11838,9 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
          * with error -TARGET_EINTR and if arg4 is not NULL and arg2 is not
          * TIMER_ABSTIME, it returns the remaining unslept time in arg4.
          */
-        if (ret == -TARGET_EINTR && arg4 && arg2 != TIMER_ABSTIME) {
-            host_to_target_timespec(arg4, &ts);
+        if (ret == -TARGET_EINTR && arg4 && arg2 != TIMER_ABSTIME &&
+            host_to_target_timespec(arg4, &ts)) {
+              return -TARGET_EFAULT;
         }
 
         return ret;
-- 
2.25.1

Re: [PATCH] linux-user: Fix 'clock_nanosleep()' implementation

Posted by Laurent Vivier 3 years, 9 months ago

Le 27/07/2020 à 22:13, Filip Bozuta a écrit :
> Implementation of syscall 'clock_nanosleep()' in 'syscall.c' uses
> functions 'target_to_host_timespec()' and 'host_to_target_timespec()'
> to transfer the value of 'struct timespec' between target and host.
> However, the implementation doesn't check whether this conversion
> succeeds and thus can return an unaproppriate error instead of 'EFAULT'
> that is expected. This was confirmed with the modified LTP test suite
> where testcases with bad 'struct timespec' adress for 'clock_nanosleep()'
> were added. This modified LTP suite can be found at:
> https://github.com/bozutaf/ltp
> 
> (Patch with this new test case will be sent to LTP mailing list soon)
> 
> Signed-off-by: Filip Bozuta <Filip.Bozuta@syrmia.com>
> ---
>  linux-user/syscall.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f5c4f6b95d..9f06dde947 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11828,7 +11828,9 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>      case TARGET_NR_clock_nanosleep:
>      {
>          struct timespec ts;
> -        target_to_host_timespec(&ts, arg3);
> +        if (target_to_host_timespec(&ts, arg3)) {
> +            return -TARGET_EFAULT;
> +        }
>          ret = get_errno(safe_clock_nanosleep(arg1, arg2,
>                                               &ts, arg4 ? &ts : NULL));
>          /*
> @@ -11836,8 +11838,9 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>           * with error -TARGET_EINTR and if arg4 is not NULL and arg2 is not
>           * TIMER_ABSTIME, it returns the remaining unslept time in arg4.
>           */
> -        if (ret == -TARGET_EINTR && arg4 && arg2 != TIMER_ABSTIME) {
> -            host_to_target_timespec(arg4, &ts);
> +        if (ret == -TARGET_EINTR && arg4 && arg2 != TIMER_ABSTIME &&
> +            host_to_target_timespec(arg4, &ts)) {
> +              return -TARGET_EFAULT;
>          }
>  
>          return ret;
> 

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

Re: [PATCH] linux-user: Fix 'clock_nanosleep()' implementation

Posted by Laurent Vivier 3 years, 8 months ago

Le 27/07/2020 à 22:13, Filip Bozuta a écrit :
> Implementation of syscall 'clock_nanosleep()' in 'syscall.c' uses
> functions 'target_to_host_timespec()' and 'host_to_target_timespec()'
> to transfer the value of 'struct timespec' between target and host.
> However, the implementation doesn't check whether this conversion
> succeeds and thus can return an unaproppriate error instead of 'EFAULT'
> that is expected. This was confirmed with the modified LTP test suite
> where testcases with bad 'struct timespec' adress for 'clock_nanosleep()'
> were added. This modified LTP suite can be found at:
> https://github.com/bozutaf/ltp
> 
> (Patch with this new test case will be sent to LTP mailing list soon)
> 
> Signed-off-by: Filip Bozuta <Filip.Bozuta@syrmia.com>
> ---
>  linux-user/syscall.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f5c4f6b95d..9f06dde947 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11828,7 +11828,9 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>      case TARGET_NR_clock_nanosleep:
>      {
>          struct timespec ts;
> -        target_to_host_timespec(&ts, arg3);
> +        if (target_to_host_timespec(&ts, arg3)) {
> +            return -TARGET_EFAULT;
> +        }
>          ret = get_errno(safe_clock_nanosleep(arg1, arg2,
>                                               &ts, arg4 ? &ts : NULL));
>          /*
> @@ -11836,8 +11838,9 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>           * with error -TARGET_EINTR and if arg4 is not NULL and arg2 is not
>           * TIMER_ABSTIME, it returns the remaining unslept time in arg4.
>           */
> -        if (ret == -TARGET_EINTR && arg4 && arg2 != TIMER_ABSTIME) {
> -            host_to_target_timespec(arg4, &ts);
> +        if (ret == -TARGET_EINTR && arg4 && arg2 != TIMER_ABSTIME &&
> +            host_to_target_timespec(arg4, &ts)) {
> +              return -TARGET_EFAULT;
>          }
>  
>          return ret;
> 

Applied to my linux-user-for-5.2 branch.

Thanks,
Laurent