On Wed, Oct 12, 2022, at 5:53 AM, Nicholas Piggin wrote:
> powerpc 32-bit system call (and function) calling convention for 64-bit
> arguments requires the next available odd-pair (two sequential registers
> with the first being odd-numbered) from the standard register argument
> allocation.
>
> The first argument register is r3, so a 64-bit argument that appears at
> an even position in the argument list must skip a register (unless there
> were preceeding 64-bit arguments, which might throw things off). This
> requires non-standard compat definitions to deal with the holes in the
> argument register allocation.
>
> With pt_regs syscall wrappers which use a standard mapper to map pt_regs
> GPRs to function arguments, 32-bit kernels hit the same basic problem,
> the standard definitions don't cope with the unused argument registers.
>
> Fix this by having 32-bit kernels share those syscall definitions with
> compat.
>
> Thanks to Jason for spending a lot of time finding and bisecting this and
> developing a trivial reproducer. The perfect bug report.
>
> Reported-by: Jason A. Donenfeld <ja...@zx2c4.com>
> Signed-off-by: Nicholas Piggin <npig...@gmail.com>
Reviewed-by: Arnd Bergmann <a...@arndb.de>
This looks like a good approach to fix the regression. Comments
below only for additional thoughts, don't let that hold up
merging.
> +#ifdef CONFIG_PPC32
> +long sys_ppc_pread64(unsigned int fd,
> + char __user *ubuf, compat_size_t count,
> + u32 reg6, u32 pos1, u32 pos2);
> +long sys_ppc_pwrite64(unsigned int fd,
> + const char __user *ubuf, compat_size_t count,
> + u32 reg6, u32 pos1, u32 pos2);
> +long sys_ppc_readahead(int fd, u32 r4,
> + u32 offset1, u32 offset2, u32 count);
> +long sys_ppc_truncate64(const char __user *path, u32 reg4,
> + unsigned long len1, unsigned long len2);
> +long sys_ppc_ftruncate64(unsigned int fd, u32 reg4,
> + unsigned long len1, unsigned long len2);
> +long sys_ppc32_fadvise64(int fd, u32 unused, u32 offset1, u32 offset2,
> + size_t len, int advice);
> +#endif
In general, I would leave out the #ifdef here and always declare
the functions, but it doesn't really matter.
> *
> - * These routines maintain argument size conversion between 32bit and 64bit
> - * environment.
> + * 32-bit system calls with 64-bit arguments pass those in register pairs.
> + * This must be specially dealt with on 64-bit kernels. The
> compat_arg_u64_dual
> + * in generic compat syscalls is not always usable because the register
> + * pairing is constrained depending on preceeding arguments.
> + *
> + * An analogous problem exists on 32-bit kernels with
> ARCH_HAS_SYSCALL_WRAPPER,
> + * the defined system call functions take the pt_regs as an argument, and
> there
> + * is a mapping macro which maps registers to arguments
> + * (SC_POWERPC_REGS_TO_ARGS) which also does not deal with these 64-bit
> + * arguments.
> + *
> + * This file contains these system calls.
It would be nice to eventually move these next to the regular system
call definitions, with more generic naming and #ifdef checks. It looks
like these are the exact same ones that we have in
arch/arm64/kernel/sys32.c and arch/mips/kernel/linux32.c,
while the other five (x86, s390, sparc, riscv, parisc) use the
version without padding that was recently added as the generic
compat syscall set.
> @@ -47,7 +57,17 @@
> #include <asm/syscalls.h>
> #include <asm/switch_to.h>
>
> -COMPAT_SYSCALL_DEFINE6(ppc_pread64,
> +#ifdef CONFIG_PPC32
> +#define PPC32_SYSCALL_DEFINE4 SYSCALL_DEFINE4
> +#define PPC32_SYSCALL_DEFINE5 SYSCALL_DEFINE5
> +#define PPC32_SYSCALL_DEFINE6 SYSCALL_DEFINE6
> +#else
> +#define PPC32_SYSCALL_DEFINE4 COMPAT_SYSCALL_DEFINE4
> +#define PPC32_SYSCALL_DEFINE5 COMPAT_SYSCALL_DEFINE5
> +#define PPC32_SYSCALL_DEFINE6 COMPAT_SYSCALL_DEFINE6
> +#endif
I'm fairly sure what you do here is correct, but I am not convinced
we actually need this as long as none of the syscalls take a signed
'long' argument that requires sign-extension for compat mode but
not native 32-bit kernels.
If we add a generic version, it would be nice to always just
use SYSCALL_DEFINEx instead of COMPAT_SYSCALL_DEFINEx. This would
also simplify the syscall table. Do you see a possible problem with
that?
Arnd
