[PATCH] x86/entry/ia32: Ensure s32 is sign extended to s64

Presently ia32 registers stored in ptregs are unconditionally cast to
unsigned int by the ia32 stub. They are then cast to long when passed
to __se_sys*, but will not be sign extended.

This takes the sign of the syscall argument into account in the ia32
stub. It still casts to unsigned int to avoid implementation specific
behavior. However then casts to int or unsigned int as necessary. So
that the following cast to long sign extends the value.

This fixes the io_pgetevents02 LTP test when compiled with
-m32. Presently the systemcall io_pgetevents_time64 unexpectedly
accepts -1 for the maximum number of events. It doesn't appear other
systemcalls with signed arguments are effected because they all have
compat variants defined and wired up. A less general solution is to
wire up the systemcall:
https://lore.kernel.org/ltp/20210921130127.24131-1-rpalethorpe@suse.com/

Fixes: ebeb8c82ffaf ("syscalls/x86: Use 'struct pt_regs' based syscall calling for IA32_EMULATION and x32")
Signed-off-by: Richard Palethorpe <redacted>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/s390/include/asm/syscall_wrapper.h |  2 --
 arch/x86/include/asm/syscall_wrapper.h  | 25 +++++++++++++++++++++----
 include/linux/syscalls.h                |  1 +
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/arch/s390/include/asm/syscall_wrapper.h b/arch/s390/include/asm/syscall_wrapper.h
index ad2c996e7e93..25ab58b0ded1 100644
--- a/arch/s390/include/asm/syscall_wrapper.h
+++ b/arch/s390/include/asm/syscall_wrapper.h

@@ -7,8 +7,6 @@
 #ifndef _ASM_S390_SYSCALL_WRAPPER_H
 #define _ASM_S390_SYSCALL_WRAPPER_H
 
-#define __SC_TYPE(t, a) t
-
 #define SYSCALL_PT_ARG6(regs, m, t1, t2, t3, t4, t5, t6)\
 	SYSCALL_PT_ARG5(regs, m, t1, t2, t3, t4, t5),	\
 		m(t6, (regs->gprs[7]))

diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h
index 6a2827d0681f..811139a82b13 100644
--- a/arch/x86/include/asm/syscall_wrapper.h
+++ b/arch/x86/include/asm/syscall_wrapper.h

@@ -58,12 +58,29 @@ extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
 		,,regs->di,,regs->si,,regs->dx				\
 		,,regs->r10,,regs->r8,,regs->r9)			\
 
+
+/* SYSCALL_PT_ARGS is Adapted from s390x */
+#define SYSCALL_PT_ARG6(m, t1, t2, t3, t4, t5, t6)			\
+	SYSCALL_PT_ARG5(m, t1, t2, t3, t4, t5), m(t6, (regs->bp))
+#define SYSCALL_PT_ARG5(m, t1, t2, t3, t4, t5)				\
+	SYSCALL_PT_ARG4(m, t1, t2, t3, t4),  m(t5, (regs->di))
+#define SYSCALL_PT_ARG4(m, t1, t2, t3, t4)				\
+	SYSCALL_PT_ARG3(m, t1, t2, t3),  m(t4, (regs->si))
+#define SYSCALL_PT_ARG3(m, t1, t2, t3)					\
+	SYSCALL_PT_ARG2(m, t1, t2), m(t3, (regs->dx))
+#define SYSCALL_PT_ARG2(m, t1, t2)					\
+	SYSCALL_PT_ARG1(m, t1), m(t2, (regs->cx))
+#define SYSCALL_PT_ARG1(m, t1) m(t1, (regs->bx))
+#define SYSCALL_PT_ARGS(x, ...) SYSCALL_PT_ARG##x(__VA_ARGS__)
+
+#define __SC_COMPAT_CAST(t, a)						\
+	(__typeof(__builtin_choose_expr(__TYPE_IS_L(t), 0, 0U)))	\
+	(unsigned int)a
+
 /* Mapping of registers to parameters for syscalls on i386 */
 #define SC_IA32_REGS_TO_ARGS(x, ...)					\
-	__MAP(x,__SC_ARGS						\
-	      ,,(unsigned int)regs->bx,,(unsigned int)regs->cx		\
-	      ,,(unsigned int)regs->dx,,(unsigned int)regs->si		\
-	      ,,(unsigned int)regs->di,,(unsigned int)regs->bp)
+	SYSCALL_PT_ARGS(x, __SC_COMPAT_CAST,				\
+			__MAP(x, __SC_TYPE, __VA_ARGS__))		\
 
 #define __SYS_STUB0(abi, name)						\
 	long __##abi##_##name(const struct pt_regs *regs);		\

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 69c9a7010081..a492276a11f1 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h

@@ -122,6 +122,7 @@ enum landlock_rule_type;
 #define __TYPE_IS_LL(t) (__TYPE_AS(t, 0LL) || __TYPE_AS(t, 0ULL))
 #define __SC_LONG(t, a) __typeof(__builtin_choose_expr(__TYPE_IS_LL(t), 0LL, 0L)) a
 #define __SC_CAST(t, a)	(__force t) a
+#define __SC_TYPE(t, a)	t
 #define __SC_ARGS(t, a)	a
 #define __SC_TEST(t, a) (void)BUILD_BUG_ON_ZERO(!__TYPE_IS_LL(t) && sizeof(t) > sizeof(long))
 

-- 
2.31.1

On Mon, Sep 27, 2021 at 6:21 PM Richard Palethorpe [off-list ref] wrote:

Presently ia32 registers stored in ptregs are unconditionally cast to
unsigned int by the ia32 stub. They are then cast to long when passed
to __se_sys*, but will not be sign extended.

This takes the sign of the syscall argument into account in the ia32
stub. It still casts to unsigned int to avoid implementation specific
behavior. However then casts to int or unsigned int as necessary. So
that the following cast to long sign extends the value.

This fixes the io_pgetevents02 LTP test when compiled with
-m32. Presently the systemcall io_pgetevents_time64 unexpectedly
accepts -1 for the maximum number of events. It doesn't appear other
systemcalls with signed arguments are effected because they all have
compat variants defined and wired up. A less general solution is to
wire up the systemcall:
https://lore.kernel.org/ltp/20210921130127.24131-1-rpalethorpe@suse.com/

Fixes: ebeb8c82ffaf ("syscalls/x86: Use 'struct pt_regs' based syscall calling for IA32_EMULATION and x32")
Signed-off-by: Richard Palethorpe <redacted>
Suggested-by: Arnd Bergmann <arnd@arndb.de>

Looks good to me, thanks for following through with this part, and for
checking the other syscalls!

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

I've added this to my randconfig build tree as well, to see if
it causes any unexpected compile-time issues, though I
don't expect any problems here.

There are a few things that I think we should do as a follow-up:

- do the same thing in the generic syscall wrapper, to ensure the
  other architectures also do the sign-extension.

- Fix the big-endian architectures (ppc64be, mips64be, sparc, s390
  parisc) so they pass the correct signal mask, either using your original
  approach, or by reworking the syscall to detect compat syscalls
  at runtime, killing off the separate entry point

- Go through the compat syscalls to see if any of them can be
  removed once all architectures do sign-extension correctly.

Are you motivated to help out with one or more of these as well?

       Arnd

Hello Arnd,

Arnd Bergmann [off-list ref] writes:

On Mon, Sep 27, 2021 at 6:21 PM Richard Palethorpe [off-list ref] wrote:

quoted

Presently ia32 registers stored in ptregs are unconditionally cast to
unsigned int by the ia32 stub. They are then cast to long when passed
to __se_sys*, but will not be sign extended.

This takes the sign of the syscall argument into account in the ia32
stub. It still casts to unsigned int to avoid implementation specific
behavior. However then casts to int or unsigned int as necessary. So
that the following cast to long sign extends the value.

This fixes the io_pgetevents02 LTP test when compiled with
-m32. Presently the systemcall io_pgetevents_time64 unexpectedly
accepts -1 for the maximum number of events. It doesn't appear other
systemcalls with signed arguments are effected because they all have
compat variants defined and wired up. A less general solution is to
wire up the systemcall:
https://lore.kernel.org/ltp/20210921130127.24131-1-rpalethorpe@suse.com/

Fixes: ebeb8c82ffaf ("syscalls/x86: Use 'struct pt_regs' based syscall calling for IA32_EMULATION and x32")
Signed-off-by: Richard Palethorpe <redacted>
Suggested-by: Arnd Bergmann <arnd@arndb.de>

Looks good to me, thanks for following through with this part, and for
checking the other syscalls!

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Thanks for the feedback and suggestions.

I've added this to my randconfig build tree as well, to see if
it causes any unexpected compile-time issues, though I
don't expect any problems here.

There are a few things that I think we should do as a follow-up:

- do the same thing in the generic syscall wrapper, to ensure the
  other architectures also do the sign-extension.

- Fix the big-endian architectures (ppc64be, mips64be, sparc, s390
  parisc) so they pass the correct signal mask, either using your original
  approach, or by reworking the syscall to detect compat syscalls
  at runtime, killing off the separate entry point

- Go through the compat syscalls to see if any of them can be
  removed once all architectures do sign-extension correctly.

Are you motivated to help out with one or more of these as well?

       Arnd

I am motivated. There have been a number of nasty bugs in compat
code. Including high-profile stuff like CVE-2021-22555. However also
just relatively minor things which cause tests to fail and could be
masking worse issues. I like the idea of removing as much syscall/arch
specific compat code as possible.

I also wonder whether syscalls like ftruncate64 can be generalised and
if there would be any benefit to doing so. All it is doing is merging
two u32 args into an s64 arg.

-- 
Thank you,
Richard.