linux-kernel - Re: [5.2 REGRESSION] Generic vDSO breaks seccomp-enabled userspace on i386

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.1907281225410.1791@nanos.tec.linutronix.de>
Date:   Sun, 28 Jul 2019 12:30:08 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Arnd Bergmann <arnd@...db.de>
cc:     Andy Lutomirski <luto@...nel.org>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
        Paul Bolle <pebolle@...cali.nl>
Subject: Re: [5.2 REGRESSION] Generic vDSO breaks seccomp-enabled userspace
 on i386

On Sun, 28 Jul 2019, Arnd Bergmann wrote:
> On Sat, Jul 27, 2019 at 7:53 PM Andy Lutomirski <luto@...nel.org> wrote:
> lib/vdso/gettimeofday.c:
> static __maybe_unused int
> __cvdso_clock_gettime32(clockid_t clock, struct old_timespec32 *res)
> {
>         struct __kernel_timespec ts;
>         int ret;
> 
>         if (res == NULL)
>                 goto fallback;
> 
>         ret = __cvdso_clock_gettime(clock, &ts);
> 
>         if (ret == 0) {
>                 res->tv_sec = ts.tv_sec;
>                 res->tv_nsec = ts.tv_nsec;
>         }
> 
>         return ret;
> 
> fallback:
>         return clock_gettime_fallback(clock, (struct __kernel_timespec *)res);
> }
> 
> So we get an 'old_timespec32' pointer from user space, and cast
> it to __kernel_timespec in order to pass it to the low-level function
> that actually fills in the 64-bit structure.
>
> On a little-endian machine, the first four bytes are actually correct
> here, but this is followed by tv_nsec=0 and 8 more bytes that overwrite
> whatever comes after the user space 'timespec'. [I missed the
> typecast as an indication of a bug during my review, sorry about
> that].

Which is totally irrelevant because res is NULL and that NULL pointer check
should simply return -EFAULT, which is what the syscall fallback returns
because the pointer is NULL.

But that NULL pointer check is inconsistent anyway:

 - 64 bit does not have it and never had

 - the vdso is not capable of handling faults properly anyway. If the
   pointer is not valid, then it will segfault. So just preventing the
   segfault for NULL is silly.

I'm going to just remove it.

Thanks,

	tglx