[PATCH] Syscall arguments are unsigned long (full registers)

All syscall arguments are passed in as types of the same byte size as
unsigned long (width of full registers). Using a smaller type without a
cast may result in losing bits of information. In all other instances
apart from the ones fixed by the patch the code explicitly introduces
type casts (using, e.g., SYSCALL_DEFINE1).

While goto-cc reported these problems at build time, it is noteworthy
that the calling conventions specified in the System V AMD64 ABI do
ensure that parameters 1-6 are passed via registers, thus there is no
implied risk of misaligned stack access.

Signed-off-by: Michael Tautschnig <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jaswinder Singh <redacted>
Cc: Andi Kleen <redacted>
---
 arch/x86/include/asm/syscalls.h | 4 ++--
 arch/x86/kernel/ioport.c        | 2 +-
 arch/x86/kernel/process_64.c    | 2 +-
 include/linux/syscalls.h        | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/syscalls.h b/arch/x86/include/asm/syscalls.h
index 91dfcaf..7dc3161 100644
--- a/arch/x86/include/asm/syscalls.h
+++ b/arch/x86/include/asm/syscalls.h

@@ -17,7 +17,7 @@

 /* Common in X86_32 and X86_64 */
 /* kernel/ioport.c */
-asmlinkage long sys_ioperm(unsigned long, unsigned long, int);
+asmlinkage long sys_ioperm(unsigned long, unsigned long, unsigned long);
 asmlinkage long sys_iopl(unsigned int);

 /* kernel/ldt.c */

@@ -45,7 +45,7 @@ asmlinkage long sys_vm86(unsigned long, unsigned long);

 /* X86_64 only */
 /* kernel/process_64.c */
-asmlinkage long sys_arch_prctl(int, unsigned long);
+asmlinkage long sys_arch_prctl(unsigned long, unsigned long);

 /* kernel/sys_x86_64.c */
 asmlinkage long sys_mmap(unsigned long, unsigned long, unsigned long,

diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
index 589b319..ae8ce91 100644
--- a/arch/x86/kernel/ioport.c
+++ b/arch/x86/kernel/ioport.c

@@ -20,7 +20,7 @@
 /*
  * this changes the io permissions bitmap in the current task.
  */
-asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int turn_on)
+asmlinkage long sys_ioperm(unsigned long from, unsigned long num, unsigned long turn_on)
 {
 	struct thread_struct *t = &current->thread;
 	struct tss_struct *tss;

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 6e789ca..a4ec3e3 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c

@@ -585,7 +585,7 @@ long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
 	return ret;
 }

-long sys_arch_prctl(int code, unsigned long addr)
+long sys_arch_prctl(unsigned long code, unsigned long addr)
 {
 	return do_arch_prctl(current, code, addr);
 }

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d022390..ca3c03d 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h

@@ -492,7 +492,7 @@ asmlinkage long sys_pipe2(int __user *fildes, int flags);
 asmlinkage long sys_dup(unsigned int fildes);
 asmlinkage long sys_dup2(unsigned int oldfd, unsigned int newfd);
 asmlinkage long sys_dup3(unsigned int oldfd, unsigned int newfd, int flags);
-asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int on);
+asmlinkage long sys_ioperm(unsigned long from, unsigned long num, unsigned long on);
 asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd,
 				unsigned long arg);
 asmlinkage long sys_flock(unsigned int fd, unsigned int cmd);
--

2.7.3.AMZN

On Mon, Jul 04, 2016 at 01:52:58PM +0000, Tautschnig, Michael wrote:

All syscall arguments are passed in as types of the same byte size as
unsigned long (width of full registers). Using a smaller type without a
cast may result in losing bits of information. In all other instances
apart from the ones fixed by the patch the code explicitly introduces
type casts (using, e.g., SYSCALL_DEFINE1).

While goto-cc reported these problems at build time, it is noteworthy
that the calling conventions specified in the System V AMD64 ABI do
ensure that parameters 1-6 are passed via registers, thus there is no
implied risk of misaligned stack access.

Does this actually fix anything?

It seems a big dangerous to me, potentially breaking some existing
binaries that rely on these arguments being truncated.

-Andi

Thanks a lot for the immediate feedback.

On 4 Jul 2016, at 16:28, Andi Kleen [off-list ref] wrote:

On Mon, Jul 04, 2016 at 01:52:58PM +0000, Tautschnig, Michael wrote:

quoted

All syscall arguments are passed in as types of the same byte size as
unsigned long (width of full registers). Using a smaller type without a
cast may result in losing bits of information. In all other instances
apart from the ones fixed by the patch the code explicitly introduces
type casts (using, e.g., SYSCALL_DEFINE1).

While goto-cc reported these problems at build time, it is noteworthy
that the calling conventions specified in the System V AMD64 ABI do
ensure that parameters 1-6 are passed via registers, thus there is no
implied risk of misaligned stack access.

Does this actually fix anything?

It will ensure the behaviour on 32 and 64-bit systems is consistent, i.e.,
no truncation occurs. This is to ensure that future uses of these syscalls
do not face surprises.

It seems a big dangerous to me, potentially breaking some existing
binaries that rely on these arguments being truncated.

Would an analysis of all current call sites be of help? It seems impossible
to tell whether any modules outside the kernel tree using this functionality
rely on the (seemingly broken) behaviour.

Of course I could also provide a patch that introduces explicit type casts
to document the truncation.

Best,
Michael

On Monday, July 4, 2016 2:47:10 PM CEST Tautschnig, Michael wrote:

Thanks a lot for the immediate feedback.

quoted

On 4 Jul 2016, at 16:28, Andi Kleen [off-list ref] wrote:

On Mon, Jul 04, 2016 at 01:52:58PM +0000, Tautschnig, Michael wrote:

quoted

All syscall arguments are passed in as types of the same byte size as
unsigned long (width of full registers). Using a smaller type without a
cast may result in losing bits of information. In all other instances
apart from the ones fixed by the patch the code explicitly introduces
type casts (using, e.g., SYSCALL_DEFINE1).

While goto-cc reported these problems at build time, it is noteworthy
that the calling conventions specified in the System V AMD64 ABI do
ensure that parameters 1-6 are passed via registers, thus there is no
implied risk of misaligned stack access.

Does this actually fix anything?

It will ensure the behaviour on 32 and 64-bit systems is consistent, i.e.,
no truncation occurs. This is to ensure that future uses of these syscalls
do not face surprises.

It looks to me like you are introducing a truncation, not removing
one as your comment suggests:

long do_arch_prctl(struct task_struct *task, int code, unsigned long addr);

-long sys_arch_prctl(int code, unsigned long addr)
+long sys_arch_prctl(unsigned long code, unsigned long addr)
 {
        return do_arch_prctl(current, code, addr);
 }

This is the same truncation that we do with SYSCALL_DEFINE2(),
clearing the top 32 bits of the 'code' parameter to ensure that
user space doesn't pass data unexpectedly.

That change seems reasonable, but why not just use SYSCALL_DEFINE2()
directly for consistency with the other syscalls?

	Arnd

On 4 Jul 2016, at 16:59, Arnd Bergmann [off-list ref] wrote:

On Monday, July 4, 2016 2:47:10 PM CEST Tautschnig, Michael wrote:

quoted

Thanks a lot for the immediate feedback.

quoted

On 4 Jul 2016, at 16:28, Andi Kleen [off-list ref] wrote:

On Mon, Jul 04, 2016 at 01:52:58PM +0000, Tautschnig, Michael wrote:

quoted

All syscall arguments are passed in as types of the same byte size as
unsigned long (width of full registers). Using a smaller type without a
cast may result in losing bits of information. In all other instances
apart from the ones fixed by the patch the code explicitly introduces
type casts (using, e.g., SYSCALL_DEFINE1).

While goto-cc reported these problems at build time, it is noteworthy
that the calling conventions specified in the System V AMD64 ABI do
ensure that parameters 1-6 are passed via registers, thus there is no
implied risk of misaligned stack access.

Does this actually fix anything?

It will ensure the behaviour on 32 and 64-bit systems is consistent, i.e.,
no truncation occurs. This is to ensure that future uses of these syscalls

   ^^^ no *hidden*

quoted

do not face surprises.

[...]

This is the same truncation that we do with SYSCALL_DEFINE2(),
clearing the top 32 bits of the 'code' parameter to ensure that
user space doesn't pass data unexpectedly.

That change seems reasonable, but why not just use SYSCALL_DEFINE2()
directly for consistency with the other syscalls?

Happy to provide such an updated patch; Andi seemed less confident this should
be going ahead?

Best,
Michael

On July 4, 2016 6:52:58 AM PDT, "Tautschnig, Michael" [off-list ref] wrote:

Wrong.  Syscall arguments aren't necessarily full registers, and on x86 truncation is already done by the callee, so we don't need any special handing.  Some other architectures have other constraints.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

On 4 Jul 2016, at 20:27, H. Peter Anvin [off-list ref] wrote:

On July 4, 2016 6:52:58 AM PDT, "Tautschnig, Michael" [off-list ref] wrote:

quoted

All syscall arguments are passed in as types of the same byte size as
unsigned long (width of full registers). Using a smaller type without a
cast may result in losing bits of information. In all other instances
apart from the ones fixed by the patch the code explicitly introduces
type casts (using, e.g., SYSCALL_DEFINE1).

While goto-cc reported these problems at build time, it is noteworthy
that the calling conventions specified in the System V AMD64 ABI do
ensure that parameters 1-6 are passed via registers, thus there is no
implied risk of misaligned stack access.

[...]

Wrong.  Syscall arguments aren't necessarily full registers, and on x86 truncation is already done by the callee, so we don't need any special handing.  Some other architectures have other constraints. 

Ok - I'm assuming I have thus misunderstood eb974c62565072e10c1422eb3205f5b611dd99a1 ? Supposedly all those SYSCALL_DEFINEx are required for other architectures only?

Best,
Michael

On July 4, 2016 1:13:21 PM PDT, "Tautschnig, Michael" [off-list ref] wrote:

quoted

On 4 Jul 2016, at 20:27, H. Peter Anvin [off-list ref] wrote:

On July 4, 2016 6:52:58 AM PDT, "Tautschnig, Michael"

[off-list ref] wrote:

quoted

quoted

All syscall arguments are passed in as types of the same byte size

as

quoted

quoted

unsigned long (width of full registers). Using a smaller type

without a

quoted

quoted

cast may result in losing bits of information. In all other

instances

quoted

quoted

apart from the ones fixed by the patch the code explicitly

introduces

quoted

quoted

type casts (using, e.g., SYSCALL_DEFINE1).

While goto-cc reported these problems at build time, it is

noteworthy

quoted

quoted

that the calling conventions specified in the System V AMD64 ABI do
ensure that parameters 1-6 are passed via registers, thus there is

no

quoted

quoted

implied risk of misaligned stack access.

[...]

quoted

Wrong.  Syscall arguments aren't necessarily full registers, and on

x86 truncation is already done by the callee, so we don't need any
special handing.  Some other architectures have other constraints. 

Ok - I'm assuming I have thus misunderstood
eb974c62565072e10c1422eb3205f5b611dd99a1 ? Supposedly all those
SYSCALL_DEFINEx are required for other architectures only?

Best,
Michael

That, and tracing.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.