Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Anton Blanchard <anton@ozlabs.org>, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org
Cc:     Nicholas Piggin <npiggin@gmail.com>, christophe.leroy@c-s.fr,
        benh@kernel.crashing.org, paulus@ozlabs.org
Subject: Re: [PATCH] powerpc/vdso: Fix multiple issues with sys_call_table
In-Reply-To: <20200306135705.7f80fcad@kryten.localdomain>
References: <20200306135705.7f80fcad@kryten.localdomain>
Date:   Thu, 19 Mar 2020 12:10:03 +1100
Message-ID: <87pnd9duac.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Anton Blanchard <anton@ozlabs.org> writes:
> The VDSO exports a bitmap of valid syscalls. vdso_setup_syscall_map()
> sets this up, but there are both little and big endian bugs. The issue
> is with:
>
>        if (sys_call_table[i] != sys_ni_syscall)
>
> On little endian, instead of comparing pointers to the two functions,
> we compare the first two instructions of each function. If a function
> happens to have the same first two instructions as sys_ni_syscall, then
> we have a spurious match and mark the instruction as not implemented.
> Fix this by removing the inline declarations.
>
> On big endian we have a further issue where sys_ni_syscall is a function
> descriptor and sys_call_table[] holds pointers to the instruction text.
> Fix this by using dereference_kernel_function_descriptor().
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Anton Blanchard <anton@ozlabs.org>

That's some pretty epic breakage.

Is it even worth keeping, or should we just rip it out and declare that
the syscall map is junk? Userspace can hardly rely on it given it's been
this broken for so long.

If not it would be really nice to have a selftest of this stuff so we
can verify it works and not break it again in future.

cheers

> ---
> diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
> index b9a108411c0d..d186b729026e 100644
> --- a/arch/powerpc/kernel/vdso.c
> +++ b/arch/powerpc/kernel/vdso.c
> @@ -17,6 +17,7 @@
>  #include <linux/elf.h>
>  #include <linux/security.h>
>  #include <linux/memblock.h>
> +#include <linux/syscalls.h>
>  
>  #include <asm/pgtable.h>
>  #include <asm/processor.h>
> @@ -30,6 +31,7 @@
>  #include <asm/vdso.h>
>  #include <asm/vdso_datapage.h>
>  #include <asm/setup.h>
> +#include <asm/syscall.h>
>  
>  #undef DEBUG
>  
> @@ -644,19 +646,16 @@ static __init int vdso_setup(void)
>  static void __init vdso_setup_syscall_map(void)
>  {
>  	unsigned int i;
> -	extern unsigned long *sys_call_table;
> -#ifdef CONFIG_PPC64
> -	extern unsigned long *compat_sys_call_table;
> -#endif
> -	extern unsigned long sys_ni_syscall;
> +	unsigned long ni_syscall;
>  
> +	ni_syscall = (unsigned long)dereference_kernel_function_descriptor(sys_ni_syscall);
>  
>  	for (i = 0; i < NR_syscalls; i++) {
>  #ifdef CONFIG_PPC64
> -		if (sys_call_table[i] != sys_ni_syscall)
> +		if (sys_call_table[i] != ni_syscall)
>  			vdso_data->syscall_map_64[i >> 5] |=
>  				0x80000000UL >> (i & 0x1f);
> -		if (compat_sys_call_table[i] != sys_ni_syscall)
> +		if (compat_sys_call_table[i] != ni_syscall)
>  			vdso_data->syscall_map_32[i >> 5] |=
>  				0x80000000UL >> (i & 0x1f);
>  #else /* CONFIG_PPC64 */