Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[Andreas Schwab]

On Okt 19 2019, Christophe Leroy <christophe.leroy@c-s.fr> wrote:

> Hi Nathan,
>
> While trying to switch powerpc VDSO to C version of gettimeofday(), I'm
> getting the following kind of error with vdsotest:
>
> passing NULL to clock_getres (VDSO): terminated by unexpected signal 11
>
> Looking at commit a9446a906f52 ("lib/vdso/32: Remove inconsistent NULL
> pointer checks"), it seems that signal 11 is expected when passing NULL
> pointer.
>
> Any plan to fix vdsotest ?

Passing NULL to clock_getres is valid, and required to return
successfully if the clock id is valid.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[Christophe Leroy]

Le 19/10/2019 à 21:18, Andreas Schwab a écrit :
> On Okt 19 2019, Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> 
>> Hi Nathan,
>>
>> While trying to switch powerpc VDSO to C version of gettimeofday(), I'm
>> getting the following kind of error with vdsotest:
>>
>> passing NULL to clock_getres (VDSO): terminated by unexpected signal 11
>>
>> Looking at commit a9446a906f52 ("lib/vdso/32: Remove inconsistent NULL
>> pointer checks"), it seems that signal 11 is expected when passing NULL
>> pointer.
>>
>> Any plan to fix vdsotest ?
> 
> Passing NULL to clock_getres is valid, and required to return
> successfully if the clock id is valid.
> 

Do you mean the following commit is wrong ?

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/lib/vdso?id=a9446a906f52292c52ecbd5be78eaa4d8395756c

Christophe

Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[Andreas Schwab]

On Okt 20 2019, Christophe Leroy <christophe.leroy@c-s.fr> wrote:

> Le 19/10/2019 à 21:18, Andreas Schwab a écrit :
>> On Okt 19 2019, Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>
>>> Hi Nathan,
>>>
>>> While trying to switch powerpc VDSO to C version of gettimeofday(), I'm
>>> getting the following kind of error with vdsotest:
>>>
>>> passing NULL to clock_getres (VDSO): terminated by unexpected signal 11
>>>
>>> Looking at commit a9446a906f52 ("lib/vdso/32: Remove inconsistent NULL
>>> pointer checks"), it seems that signal 11 is expected when passing NULL
>>> pointer.
>>>
>>> Any plan to fix vdsotest ?
>>
>> Passing NULL to clock_getres is valid, and required to return
>> successfully if the clock id is valid.
>>
>
> Do you mean the following commit is wrong ?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/lib/vdso?id=a9446a906f52292c52ecbd5be78eaa4d8395756c

If it causes a valid call to clock_getres to fail, then yes.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[Christophe Leroy]

Adding Thomas to the discussion as the commit is from him.

Le 20/10/2019 à 11:53, Andreas Schwab a écrit :
> On Okt 20 2019, Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> 
>> Le 19/10/2019 à 21:18, Andreas Schwab a écrit :
>>> On Okt 19 2019, Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>>
>>>> Hi Nathan,
>>>>
>>>> While trying to switch powerpc VDSO to C version of gettimeofday(), I'm
>>>> getting the following kind of error with vdsotest:
>>>>
>>>> passing NULL to clock_getres (VDSO): terminated by unexpected signal 11
>>>>
>>>> Looking at commit a9446a906f52 ("lib/vdso/32: Remove inconsistent NULL
>>>> pointer checks"), it seems that signal 11 is expected when passing NULL
>>>> pointer.
>>>>
>>>> Any plan to fix vdsotest ?
>>>
>>> Passing NULL to clock_getres is valid, and required to return
>>> successfully if the clock id is valid.
>>>
>>
>> Do you mean the following commit is wrong ?
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/lib/vdso?id=a9446a906f52292c52ecbd5be78eaa4d8395756c
> 
> If it causes a valid call to clock_getres to fail, then yes.
> 
> Andreas.
> 

Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[Thomas Gleixner]

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

On Sun, 20 Oct 2019, Christophe Leroy wrote:
> Adding Thomas to the discussion as the commit is from him.
> 
> Le 20/10/2019 à 11:53, Andreas Schwab a écrit :
> > On Okt 20 2019, Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> > 
> > > Le 19/10/2019 à 21:18, Andreas Schwab a écrit :
> > > > On Okt 19 2019, Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> > > > 
> > > > > Hi Nathan,
> > > > > 
> > > > > While trying to switch powerpc VDSO to C version of gettimeofday(),
> > > > > I'm
> > > > > getting the following kind of error with vdsotest:
> > > > > 
> > > > > passing NULL to clock_getres (VDSO): terminated by unexpected signal
> > > > > 11
> > > > > 
> > > > > Looking at commit a9446a906f52 ("lib/vdso/32: Remove inconsistent NULL
> > > > > pointer checks"), it seems that signal 11 is expected when passing
> > > > > NULL
> > > > > pointer.
> > > > > 
> > > > > Any plan to fix vdsotest ?
> > > > 
> > > > Passing NULL to clock_getres is valid, and required to return
> > > > successfully if the clock id is valid.
> > > > 
> > > 
> > > Do you mean the following commit is wrong ?
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/lib/vdso?id=a9446a906f52292c52ecbd5be78eaa4d8395756c
> > 
> > If it causes a valid call to clock_getres to fail, then yes.

clock_getres(NULL) is hardly valid.

Fact is that 64bit did never check the pointer and unconditionally let the
NULL pointer dereference happen.

Also as documented in the change log, the vdso _cannot_ ever be fully
equivalent to the syscall.

The simple example:

    struct timespec *ts = (struct timespec *) 0xFF;

    clock_getres(clock, ts);

will fault in the VDSO, but not in the syscall.

VDSO can never ever guarantee that any of the clock_* functions will return
EFAULT if the provided pointer points outside of the accessible address
space, is mapped RO etc.

So special casing

    clock_getres(clock, NULL);

just to make a test case happy is a pointless exercise which does not make
any sense at all.

Thanks,

	tglx

Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[Andreas Schwab]

On Okt 20 2019, Thomas Gleixner <tglx@linutronix.de> wrote:

> clock_getres(NULL) is hardly valid.

Of course not, it lacks a parameter.

> So special casing
>
>     clock_getres(clock, NULL);
>
> just to make a test case happy is a pointless exercise which does not make
> any sense at all.

POSIX requires it to work.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[Thomas Gleixner]

On Sun, 20 Oct 2019, Andreas Schwab wrote:

> On Okt 20 2019, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > clock_getres(NULL) is hardly valid.
> 
> Of course not, it lacks a parameter.

You know exactly what I mean.
 
> > So special casing
> >
> >     clock_getres(clock, NULL);
> >
> > just to make a test case happy is a pointless exercise which does not make
> > any sense at all.
> 
> POSIX requires it to work.

POSIX does not mention anything about the validity of the pointer handed to
clock_getres().

https://pubs.opengroup.org/onlinepubs/9699919799/

 "The clock_getres(), clock_gettime(), and clock_settime() functions shall fail if:

   [EINVAL]
     The clock_id argument does not specify a known clock."

EINVAL is the only documented error code for clock_getres(). 

Again. The VDSO is not a syscall and therefore neither address space
validation of the supplied pointer nor catching an invalid access works
like it does in a syscall.

The only invalid pointer it can catch is NULL, but that's ONE case out of a
gazillion. So what's the value of catching NULL and only NULL?

If you can come up with a solution to handle invalid pointers in the VDSO
gracefully, then I'm surely all ears.

If not then can you please explain why VDSO usage worked without pointer
validation since the time VDSO was invented more than a decade ago and just
now requires full invalid pointer handling?

Thanks,

	tglx

Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[Andreas Schwab]

On Okt 20 2019, Thomas Gleixner wrote:

> POSIX does not mention anything about the validity of the pointer handed to
> clock_getres().

Sure it does: "If the argument res is not NULL, the resolution of the
specified clock shall be stored in the location pointed to by res.  If
res is NULL, the clock resolution is not returned.".

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[Thomas Gleixner]

On Sun, 20 Oct 2019, Andreas Schwab wrote:
> On Okt 20 2019, Thomas Gleixner wrote:
> 
> > POSIX does not mention anything about the validity of the pointer handed to
> > clock_getres().
> 
> Sure it does: "If the argument res is not NULL, the resolution of the
> specified clock shall be stored in the location pointed to by res.  If
> res is NULL, the clock resolution is not returned.".

Sigh, that makes a lot of sense - NOT.

But for the sake of making a non-sensical specification happy we can add a
NULL pointer check for this. The interesting question is what should be
returned in this case. The kernel returns EFAULT which is probably not
POSIX compliant either.

Patches are welcome.

Thanks,

	tglx

Re: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[Andreas Schwab]

On Okt 20 2019, Thomas Gleixner wrote:

> But for the sake of making a non-sensical specification happy we can add a
> NULL pointer check for this. The interesting question is what should be
> returned in this case.

0 if the clock id is valid, EINVAL otherwise.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH] lib/vdso: Make clock_getres() POSIX compliant again

[Thomas Gleixner]

A recent commit removed the NULL pointer check from the clock_getres()
implementation causing a test case to fault.

POSIX requires an explicit NULL pointer check for clock_getres() aside of
the validity check of the clock_id argument for obscure reasons.

Add it back for both 32bit and 64bit.

Note, this is only a partial revert of the offending commit which does not
bring back the broken fallback invocation in the the 32bit compat
implementations of clock_getres() and clock_gettime().

Fixes: a9446a906f52 ("lib/vdso/32: Remove inconsistent NULL pointer checks")
Reported-by: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 lib/vdso/gettimeofday.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

--- a/lib/vdso/gettimeofday.c
+++ b/lib/vdso/gettimeofday.c
@@ -214,9 +214,10 @@ int __cvdso_clock_getres_common(clockid_
 		return -1;
 	}
 
-	res->tv_sec = 0;
-	res->tv_nsec = ns;
-
+	if (likely(res)) {
+		res->tv_sec = 0;
+		res->tv_nsec = ns;
+	}
 	return 0;
 }
 
@@ -245,7 +246,7 @@ static __maybe_unused int
 		ret = clock_getres_fallback(clock, &ts);
 #endif
 
-	if (likely(!ret)) {
+	if (likely(!ret && res)) {
 		res->tv_sec = ts.tv_sec;
 		res->tv_nsec = ts.tv_nsec;
 	}

Re: [PATCH] lib/vdso: Make clock_getres() POSIX compliant again

[Christophe Leroy]

Le 21/10/2019 à 12:07, Thomas Gleixner a écrit :
> A recent commit removed the NULL pointer check from the clock_getres()
> implementation causing a test case to fault.
> 
> POSIX requires an explicit NULL pointer check for clock_getres() aside of
> the validity check of the clock_id argument for obscure reasons.
> 
> Add it back for both 32bit and 64bit.
> 
> Note, this is only a partial revert of the offending commit which does not
> bring back the broken fallback invocation in the the 32bit compat
> implementations of clock_getres() and clock_gettime().
> 
> Fixes: a9446a906f52 ("lib/vdso/32: Remove inconsistent NULL pointer checks")
> Reported-by: Andreas Schwab <schwab@linux-m68k.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Tested-by: Christophe Leroy <christophe.leroy@c-s.fr>

> Cc: stable@vger.kernel.org
> ---
>   lib/vdso/gettimeofday.c |    9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> --- a/lib/vdso/gettimeofday.c
> +++ b/lib/vdso/gettimeofday.c
> @@ -214,9 +214,10 @@ int __cvdso_clock_getres_common(clockid_
>   		return -1;
>   	}
>   
> -	res->tv_sec = 0;
> -	res->tv_nsec = ns;
> -
> +	if (likely(res)) {
> +		res->tv_sec = 0;
> +		res->tv_nsec = ns;
> +	}
>   	return 0;
>   }
>   
> @@ -245,7 +246,7 @@ static __maybe_unused int
>   		ret = clock_getres_fallback(clock, &ts);
>   #endif
>   
> -	if (likely(!ret)) {
> +	if (likely(!ret && res)) {
>   		res->tv_sec = ts.tv_sec;
>   		res->tv_nsec = ts.tv_nsec;
>   	}
> 

RE: passing NULL to clock_getres (VDSO): terminated by unexpected signal 11

[David Laight]

From: Thomas Gleixner
> Sent: 20 October 2019 20:53
> On Sun, 20 Oct 2019, Andreas Schwab wrote:
> > On Okt 20 2019, Thomas Gleixner wrote:
> >
> > > POSIX does not mention anything about the validity of the pointer handed to
> > > clock_getres().
> >
> > Sure it does: "If the argument res is not NULL, the resolution of the
> > specified clock shall be stored in the location pointed to by res.  If
> > res is NULL, the clock resolution is not returned.".
> 
> Sigh, that makes a lot of sense - NOT.
> 
> But for the sake of making a non-sensical specification happy we can add a
> NULL pointer check for this. The interesting question is what should be
> returned in this case. The kernel returns EFAULT which is probably not
> POSIX compliant either.

The application won't see errno == EFAULT.
EFAULT gets converted to SIGSEGV (probably) in the return-to-user code path.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)