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

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

[Richard Palethorpe]

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 <rpalethorpe@suse.com>
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

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

[Arnd Bergmann]

On Mon, Sep 27, 2021 at 6:21 PM Richard Palethorpe <rpalethorpe@suse.com> 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 <rpalethorpe@suse.com>
> 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

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

[Richard Palethorpe]

Hello Arnd,

Arnd Bergmann <arnd@arndb.de> writes:

> On Mon, Sep 27, 2021 at 6:21 PM Richard Palethorpe <rpalethorpe@suse.com> 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 <rpalethorpe@suse.com>
>> 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.

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

[Florian Weimer]

* Richard Palethorpe:

> +#define __SC_COMPAT_CAST(t, a)						\
> +	(__typeof(__builtin_choose_expr(__TYPE_IS_L(t), 0, 0U)))	\
> +	(unsigned int)a

So this casts to int (triggering sign extension) if the type on the
64-bit kernel side is long?  But not in other cases (unsigned long,
pointer)?  Just double-checking.

Thanks,
Florian

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

[Arnd Bergmann]

On Mon, Sep 27, 2021 at 8:51 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Richard Palethorpe:
>
> > +#define __SC_COMPAT_CAST(t, a)                                               \
> > +     (__typeof(__builtin_choose_expr(__TYPE_IS_L(t), 0, 0U)))        \
> > +     (unsigned int)a
>
> So this casts to int (triggering sign extension) if the type on the
> 64-bit kernel side is long?  But not in other cases (unsigned long,
> pointer)?  Just double-checking.

Correct, this is the only case that is not already handled: anything smaller
than a 'long' is the same size on all architectures we support and we
ensure those are correctly sign- or zero-extended. 'unsigned long'
and any pointer are zero-extended by the entry code from 32-bit user
space to a 64-bit register in the kernel. Only signed 'long' requires
explicit sign-extending from the userspace 'long' to the kernel function
argument.

         Arnd