linux-kernel - RE: [PATCH 1/1] x86/vdso: use the right GDT_ENTRY_CPUNODE for 32-bit getcpu() on 64-bit kernel

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]

Date:   Wed, 29 Mar 2023 23:11:09 +0000
From:   "Li, Xin3" <xin3.li@...el.com>
To:     "Hansen, Dave" <dave.hansen@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "bigeasy@...utronix.de" <bigeasy@...utronix.de>,
        "Liu, Yujie" <yujie.liu@...el.com>,
        "Kang, Shan" <shan.kang@...el.com>
Subject: RE: [PATCH 1/1] x86/vdso: use the right GDT_ENTRY_CPUNODE for 32-bit
 getcpu() on 64-bit kernel

> > > > +#ifndef BUILD_VDSO32_64
> > > >  #define GDT_ENTRY_CPUNODE		28
> > > > +#else
> > > > +#define GDT_ENTRY_CPUNODE		15
> > > > +#endif
> > >
> > > Isn't this kinda a hack?
> > >
> > > First, it means that we'll now have two duplicate versions of this:
> > >
> > > 	#define GDT_ENTRY_CPUNODE		15
> > >
> > > in the same file.
> > >
> > > Second, if any other users of fake_32bit_build.h for the VDSO show
> > > up, they'll need a similar #ifdef.
> > >
> > > I think I'd much rather if we define all of the GDT_ENTRY_* macros
> > > in
> > > *one* place, then make that *one* place depend on BUILD_VDSO32_64.
> >
> > Sounds a better way, let me try.
> >
> > > Also, about the *silent* failure...  Do we not have a selftest for this somewhere?
> >
> > When lsl is used, we should check ZF which indicates whether the
> > segment limit is loaded successfully.  Seems we need to refactor
> vdso_read_cpunode() a bit.
> 
> Hi Dave,
> 
> How about the following patch to address the silent failure?
> 
> diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
> index 794f69625780..d75ce4afff5b 100644
> --- a/arch/x86/include/asm/segment.h
> +++ b/arch/x86/include/asm/segment.h
> @@ -254,7 +254,10 @@ static inline void vdso_read_cpunode(unsigned *cpu,
> unsigned *node)
>          *
>          * If RDPID is available, use it.
>          */
> -       alternative_io ("lsl %[seg],%[p]",
> +       alternative_io ("lsl %[seg],%[p]\n"
> +                       "jz 1f\n"
> +                       "mov $-1,%[p]\n"
> +                       "1:",
>                         ".byte 0xf3,0x0f,0xc7,0xf8", /* RDPID %eax/rax */
>                         X86_FEATURE_RDPID,
>                         [p] "=a" (p), [seg] "r" (__CPUNODE_SEG));
> 
> 
> And the test then reports CPU id 4095 (not a big enough #), which can be used to
> indicate a failure of the lsl instruction:

Having to say, it's a *bad* idea to use a special # to indicate an error.
But there is also no reasonable errno for getcpu() to return to its caller,
saying "we had a problem in the syscall's kernel implementation".

> 
> ~/selftests$ sudo ./run_kselftest.sh -t x86:test_vsyscall_32 TAP version 13
> 1..1
> # selftests: x86: test_vsyscall_32
> # [RUN] test gettimeofday()
> #       vDSO time offsets: 0.000028 0.000000
> # [OK]  vDSO gettimeofday()'s timeval was okay # [RUN] test time() # [OK]  vDSO
> time() is okay # [RUN] getcpu() on CPU 0
> # [FAIL]        vDSO reported CPU 4095 but should be 0
> # [FAIL]        vDSO reported node 65535 but should be 0
> # [RUN] getcpu() on CPU 1
> # [FAIL]        vDSO reported CPU 4095 but should be 1
> # [FAIL]        vDSO reported node 65535 but should be 0
> not ok 1 selftests: x86: test_vsyscall_32 # exit=1
> 
> Thanks!
>   Xin