[PATCH 1/2] linux: clock_settime: Remove check for nanoseconds validity

[PATCH 1/2] linux: clock_settime: Remove check for nanoseconds validity

[Lukasz Majewski]

The check if passed nanoseconds via struct __timespec64's *tp pointer is
also performed in the Linux kernel. Remove it from glibc to avoid
duplication.
---
 sysdeps/unix/sysv/linux/clock_settime.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c
index bda113809b..6706dbb31f 100644
--- a/sysdeps/unix/sysv/linux/clock_settime.c
+++ b/sysdeps/unix/sysv/linux/clock_settime.c
@@ -25,13 +25,6 @@
 int
 __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
 {
-  /* Make sure the time cvalue is OK.  */
-  if (! valid_nanoseconds (tp->tv_nsec))
-    {
-      __set_errno (EINVAL);
-      return -1;
-    }
-
 #ifdef __ASSUME_TIME64_SYSCALLS
 # ifndef __NR_clock_settime64
 #  define __NR_clock_settime64 __NR_clock_settime
-- 
2.20.1

[PATCH 2/2] linux: clock_settime: Return proper value when passing NULL pointer

[Lukasz Majewski]

When in __clock_settime function (__TIMESIZE != 64) the const struct
timespec's *tp pointer is NULL, the Linux kernel syscall returns
-EFAULT.
Without this patch the glibc crashes (when dereferencing NULL pointer)
as the Linux kernel syscall is not reached at all.

There is no need for such check in the __clock_settime64, as this
pointer either goes directly to Linux kernel or the pointer to local
copy is used (ts64).
---
 sysdeps/unix/sysv/linux/clock_settime.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c
index 6706dbb31f..e358a18998 100644
--- a/sysdeps/unix/sysv/linux/clock_settime.c
+++ b/sysdeps/unix/sysv/linux/clock_settime.c
@@ -51,7 +51,14 @@ __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
 int
 __clock_settime (clockid_t clock_id, const struct timespec *tp)
 {
-  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*tp);
+  struct __timespec64 ts64;
+
+  if (tp == NULL)
+    {
+      __set_errno (EFAULT);
+      return -1;
+    }
+  ts64 = valid_timespec_to_timespec64 (*tp);
 
   return __clock_settime64 (clock_id, &ts64);
 }
-- 
2.20.1

Re: [PATCH 1/2] linux: clock_settime: Remove check for nanoseconds validity

[Alistair Francis]

On Fri, Nov 8, 2019 at 7:34 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> The check if passed nanoseconds via struct __timespec64's *tp pointer is
> also performed in the Linux kernel. Remove it from glibc to avoid
> duplication.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  sysdeps/unix/sysv/linux/clock_settime.c | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c
> index bda113809b..6706dbb31f 100644
> --- a/sysdeps/unix/sysv/linux/clock_settime.c
> +++ b/sysdeps/unix/sysv/linux/clock_settime.c
> @@ -25,13 +25,6 @@
>  int
>  __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
>  {
> -  /* Make sure the time cvalue is OK.  */
> -  if (! valid_nanoseconds (tp->tv_nsec))
> -    {
> -      __set_errno (EINVAL);
> -      return -1;
> -    }
> -
>  #ifdef __ASSUME_TIME64_SYSCALLS
>  # ifndef __NR_clock_settime64
>  #  define __NR_clock_settime64 __NR_clock_settime
> --
> 2.20.1
>

Re: [PATCH 2/2] linux: clock_settime: Return proper value when passing NULL pointer

[Alistair Francis]

On Fri, Nov 8, 2019 at 7:34 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> When in __clock_settime function (__TIMESIZE != 64) the const struct
> timespec's *tp pointer is NULL, the Linux kernel syscall returns
> -EFAULT.
> Without this patch the glibc crashes (when dereferencing NULL pointer)
> as the Linux kernel syscall is not reached at all.
>
> There is no need for such check in the __clock_settime64, as this
> pointer either goes directly to Linux kernel or the pointer to local
> copy is used (ts64).

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  sysdeps/unix/sysv/linux/clock_settime.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c
> index 6706dbb31f..e358a18998 100644
> --- a/sysdeps/unix/sysv/linux/clock_settime.c
> +++ b/sysdeps/unix/sysv/linux/clock_settime.c
> @@ -51,7 +51,14 @@ __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
>  int
>  __clock_settime (clockid_t clock_id, const struct timespec *tp)
>  {
> -  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*tp);
> +  struct __timespec64 ts64;
> +
> +  if (tp == NULL)
> +    {
> +      __set_errno (EFAULT);
> +      return -1;
> +    }
> +  ts64 = valid_timespec_to_timespec64 (*tp);
>
>    return __clock_settime64 (clock_id, &ts64);
>  }
> --
> 2.20.1
>

Re: [PATCH 2/2] linux: clock_settime: Return proper value when passing NULL pointer

[Joseph Myers]

On Fri, 8 Nov 2019, Alistair Francis wrote:

> On Fri, Nov 8, 2019 at 7:34 AM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > When in __clock_settime function (__TIMESIZE != 64) the const struct
> > timespec's *tp pointer is NULL, the Linux kernel syscall returns
> > -EFAULT.
> > Without this patch the glibc crashes (when dereferencing NULL pointer)
> > as the Linux kernel syscall is not reached at all.
> >
> > There is no need for such check in the __clock_settime64, as this
> > pointer either goes directly to Linux kernel or the pointer to local
> > copy is used (ts64).
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

This patch is contrary to glibc conventions.  There is explicitly no 
guarantee of whether a segfault or EFAULT occurs when a function is called 
with invalid arguments.  There should be no explicit checks for NULL 
pointers in cases where a segfault will reliably occur otherwise and any 
existing such checks should be removed from glibc.

https://sourceware.org/glibc/wiki/Style_and_Conventions#Invalid_pointers

(And note the POSIX specification of EFAULT, "The reliable detection of 
this error cannot be guaranteed, and when not detected may result in the 
generation of a signal, indicating an address violation, which is sent to 
the process.".)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 1/2] linux: clock_settime: Remove check for nanoseconds validity

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1381 bytes --]

Dear Joseph, Paul,

> The check if passed nanoseconds via struct __timespec64's *tp pointer
> is also performed in the Linux kernel. Remove it from glibc to avoid
> duplication.
> ---
>  sysdeps/unix/sysv/linux/clock_settime.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/clock_settime.c
> b/sysdeps/unix/sysv/linux/clock_settime.c index
> bda113809b..6706dbb31f 100644 ---
> a/sysdeps/unix/sysv/linux/clock_settime.c +++
> b/sysdeps/unix/sysv/linux/clock_settime.c @@ -25,13 +25,6 @@
>  int
>  __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
>  {
> -  /* Make sure the time cvalue is OK.  */
> -  if (! valid_nanoseconds (tp->tv_nsec))
> -    {
> -      __set_errno (EINVAL);
> -      return -1;
> -    }
> -

I'm just wondering if this patch is OK, as with other patches, which 
convert time to use 64 bit syscalls we do rely on Linux kernel to
check the nanoseconds (and return proper error).

>  #ifdef __ASSUME_TIME64_SYSCALLS
>  # ifndef __NR_clock_settime64
>  #  define __NR_clock_settime64 __NR_clock_settime




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] linux: clock_settime: Remove check for nanoseconds validity

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]

Dear Joseph, Paul,

> Dear Joseph, Paul,
> 
> > The check if passed nanoseconds via struct __timespec64's *tp
> > pointer is also performed in the Linux kernel. Remove it from glibc
> > to avoid duplication.
> > ---
> >  sysdeps/unix/sysv/linux/clock_settime.c | 7 -------
> >  1 file changed, 7 deletions(-)
> > 
> > diff --git a/sysdeps/unix/sysv/linux/clock_settime.c
> > b/sysdeps/unix/sysv/linux/clock_settime.c index
> > bda113809b..6706dbb31f 100644 ---
> > a/sysdeps/unix/sysv/linux/clock_settime.c +++
> > b/sysdeps/unix/sysv/linux/clock_settime.c @@ -25,13 +25,6 @@
> >  int
> >  __clock_settime64 (clockid_t clock_id, const struct __timespec64
> > *tp) {
> > -  /* Make sure the time cvalue is OK.  */
> > -  if (! valid_nanoseconds (tp->tv_nsec))
> > -    {
> > -      __set_errno (EINVAL);
> > -      return -1;
> > -    }
> > -  
> 
> I'm just wondering if this patch is OK, as with other patches, which 
> convert time to use 64 bit syscalls we do rely on Linux kernel to
> check the nanoseconds (and return proper error).

Are there any comments regarding this patch?

> 
> >  #ifdef __ASSUME_TIME64_SYSCALLS
> >  # ifndef __NR_clock_settime64
> >  #  define __NR_clock_settime64 __NR_clock_settime  
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma@denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] linux: clock_settime: Remove check for nanoseconds validity

[Paul Eggert]

On 11/27/19 9:04 AM, Lukasz Majewski wrote:

>> a/sysdeps/unix/sysv/linux/clock_settime.c +++
>> b/sysdeps/unix/sysv/linux/clock_settime.c @@ -25,13 +25,6 @@
>>   int
>>   __clock_settime64 (clockid_t clock_id, const struct __timespec64
>> *tp) {
>> -  /* Make sure the time cvalue is OK.  */
>> -  if (! valid_nanoseconds (tp->tv_nsec))
>> -    {
>> -      __set_errno (EINVAL);
>> -      return -1;
>> -    }
>> -
> I'm just wondering if this patch is OK, as with other patches, which
> convert time to use 64 bit syscalls we do rely on Linux kernel to
> check the nanoseconds (and return proper error).

Suppose tp->tv_sec == 1 && tp->tv_nsec == -1 && !defined 
__ASSUME_TIME64_SYSCALLS && !defined __NR_clock_settime64. Then the 
current code will fail with errno == EINVAL, but with the proposed patch 
the code will succeed and set the time to 1 second after the Epoch.

Code should always check for valid nanoseconds before calling 
valid_timespec64_to_timespec with possibly-invalid input. In this 
function, the check can be done at about the same time as the 
in_time_t_range check; that'd be better than what the current code does.