| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAK8P3a0QGtjygLJUWX_1-s1vfCzE6UoOzrb+OZWwjaBdh=RpVQ@mail.gmail.com>
Date: Mon, 30 Dec 2019 13:27:39 +0100
From: Arnd Bergmann <arnd@...db.de>
To: Christophe Leroy <christophe.leroy@....fr>
Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Paul Mackerras <paulus@...ba.org>,
Michael Ellerman <mpe@...erman.id.au>,
Thomas Gleixner <tglx@...utronix.de>,
Vincenzo Frascino <vincenzo.frascino@....com>,
Andy Lutomirski <luto@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
"open list:BROADCOM NVRAM DRIVER" <linux-mips@...r.kernel.org>,
"the arch/x86 maintainers" <x86@...nel.org>
Subject: Re: [RFC PATCH v2 01/10] lib: vdso: ensure all arches have 32bit fallback
On Mon, Dec 23, 2019 at 3:31 PM Christophe Leroy
<christophe.leroy@....fr> wrote:
>
> In order to simplify next step which moves fallback call at arch
> level, ensure all arches have a 32bit fallback instead of handling
> the lack of 32bit fallback in the common code based
> on VDSO_HAS_32BIT_FALLBACK
>
> Signed-off-by: Christophe Leroy <christophe.leroy@....fr>
I like the idea of removing VDSO_HAS_32BIT_FALLBACK and ensuring
that all 32-bit architectures implement them, but we really should *not*
have any implementation calling the 64-bit syscalls.
> +static __always_inline
> +long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
> +{
> + struct __kernel_timespec ts;
> + int ret = clock_gettime_fallback(clock, &ts);
> +
> + if (likely(!ret)) {
> + _ts->tv_sec = ts.tv_sec;
> + _ts->tv_nsec = ts.tv_nsec;
> + }
> + return ret;
> +}
> +
> +static __always_inline
> +long clock_getres32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
> +{
> + struct __kernel_timespec ts;
> + int ret = clock_getres_fallback(clock, &ts);
> +
> + if (likely(!ret && _ts)) {
> + _ts->tv_sec = ts.tv_sec;
> + _ts->tv_nsec = ts.tv_nsec;
> + }
> + return ret;
> +}
Please change these to call __NR_clock_gettime and __NR_clock_getres_time
instead of __NR_clock_gettime64/__NR_clock_getres_time64 for multiple reasons.
- When doing migration between containers, the vdso may get copied into
an application running on a kernel that does not support the time64
variants, and then the fallback fails.
- When CONFIG_COMPAT_32BIT_TIME is disabled, the time32 syscalls
return -ENOSYS, and the vdso version should have the exact same behavior
to avoid surprises. In particular an application that checks clock_gettime()
to see if the time32 are in part of the kernel would get an incorrect result
here.
arch/arm64/include/asm/vdso/compat_gettimeofday.h already does this,
I think you can just copy the implementation or find a way to share it.
> diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h
> index b08f476b72b4..c41c86a07423 100644
> --- a/arch/arm64/include/asm/vdso/gettimeofday.h
> +++ b/arch/arm64/include/asm/vdso/gettimeofday.h
> @@ -66,6 +66,32 @@ int clock_getres_fallback(clockid_t _clkid, struct __kernel_timespec *_ts)
> return ret;
> }
>
> +static __always_inline
> +long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
> +{
> + struct __kernel_timespec ts;
> + int ret = clock_gettime_fallback(clock, &ts);
> +
> + if (likely(!ret)) {
> + _ts->tv_sec = ts.tv_sec;
> + _ts->tv_nsec = ts.tv_nsec;
> + }
> + return ret;
> +}
As Andy said, this makes no sense at all, nothing should ever call this on a
64-bit architecture.
> diff --git a/arch/mips/include/asm/vdso/gettimeofday.h b/arch/mips/include/asm/vdso/gettimeofday.h
> index b08825531e9f..60608e930a5c 100644
> --- a/arch/mips/include/asm/vdso/gettimeofday.h
> +++ b/arch/mips/include/asm/vdso/gettimeofday.h
> @@ -109,8 +109,6 @@ static __always_inline int clock_getres_fallback(
>
> #if _MIPS_SIM != _MIPS_SIM_ABI64
>
> -#define VDSO_HAS_32BIT_FALLBACK 1
> -
> static __always_inline long clock_gettime32_fallback(
> clockid_t _clkid,
> struct old_timespec32 *_ts)
> @@ -150,6 +148,32 @@ static __always_inline int clock_getres32_fallback(
>
> return error ? -ret : ret;
> }
> +#else
> +static __always_inline
> +long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
> +{
> + struct __kernel_timespec ts;
> + int ret = clock_gettime_fallback(clock, &ts);
> +
> + if (likely(!ret)) {
> + _ts->tv_sec = ts.tv_sec;
> + _ts->tv_nsec = ts.tv_nsec;
> + }
> + return ret;
> +}
> +
Same here.
> --- a/lib/vdso/gettimeofday.c
> +++ b/lib/vdso/gettimeofday.c
> @@ -125,13 +125,8 @@ __cvdso_clock_gettime32(clockid_t clock, struct old_timespec32 *res)
>
> ret = __cvdso_clock_gettime_common(clock, &ts);
>
> -#ifdef VDSO_HAS_32BIT_FALLBACK
> if (unlikely(ret))
> return clock_gettime32_fallback(clock, res);
> -#else
> - if (unlikely(ret))
> - ret = clock_gettime_fallback(clock, &ts);
> -#endif
>
> if (likely(!ret)) {
> res->tv_sec = ts.tv_sec;
Removing the #ifdef and the fallback seems fine. I think this is actually
required for correctness on arm32 as well. Maybe enclose the entire function in
#ifdef VDSO_HAS_CLOCK_GETTIME32
to only define it when it is called?
> @@ -238,13 +233,8 @@ __cvdso_clock_getres_time32(clockid_t clock, struct old_timespec32 *res)
>
> ret = __cvdso_clock_getres_common(clock, &ts);
>
> -#ifdef VDSO_HAS_32BIT_FALLBACK
> if (unlikely(ret))
> return clock_getres32_fallback(clock, res);
> -#else
> - if (unlikely(ret))
> - ret = clock_getres_fallback(clock, &ts);
> -#endif
The same applies to all the getres stuff of course.
Arnd
Powered by blists - more mailing lists