compat syscall args

compat syscall args

[David S. Miller]

Arnd asked:

> If sparc64 has this problem only for the fifth syscall argument, 
> does that mean that e.g. compat_sys_futex and 
> compat_sys_mq_timed{send,receive} have the same bug? If this is
> a more general, i.e. not limited to the last argument, there is a
> potential problem in lots of syscalls.

Here is the issue.  In the sparc64 C calling conventions, it is
assumed that 32-bit signed values are sign extended by the
caller.

This means that, at syscall invocation time, we have to choose
between either:

1) sign extending all syscall args for the C code, then explicitly
   zero-extending all non-signed syscall args.  This would require
   the most amount of compat layer code help.

2) zero extending all syscall args for the C code, then expliticly
   sign-extending all signed syscall args.

3) some mixture of 1 and 2

#3 is what sparc64 does, it hits the highest number of system
call arguments correctly.  Specifically we:

arg0: zero-extend
arg1: zero-extend
arg2: zero-extend
arg3: zero-extend
arg4: leave as-is
arg5: leave as-is

I remember discussing this with Andi Kleen before.

Each platform is going to behave differently in this area, so
I suppose the right thing to do really is to have the arch
specific code use little zero/sign extender stubs when necessary
so that the compat layer can assume that the args are properly
sign/zero extended already.  I guess this is how I'll fix this
up on sparc64 for now.

Comments?

Re: compat syscall args

[David S. Miller]

On Sat, 29 May 2004 12:23:19 -0700
"David S. Miller" <davem@redhat.com> wrote:

> Each platform is going to behave differently in this area, so
> I suppose the right thing to do really is to have the arch
> specific code use little zero/sign extender stubs when necessary
> so that the compat layer can assume that the args are properly
> sign/zero extended already.  I guess this is how I'll fix this
> up on sparc64 for now.

As it turns out we're taking care of this already via stubs
in arch/sparc64/kernel/sys32.S, I just need to add them for
select and futex.

Here is how I'm going to fix this on sparc64 therefore.

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/05/29 12:27:54-07:00 davem@nuts.davemloft.net 
#   [SPARC64]: compat select and futex need %o4 zero-extended.
# 
# arch/sparc64/kernel/systbls.S
#   2004/05/29 12:27:36-07:00 davem@nuts.davemloft.net +3 -3
#   [SPARC64]: compat select and futex need %o4 zero-extended.
# 
# arch/sparc64/kernel/sys32.S
#   2004/05/29 12:27:36-07:00 davem@nuts.davemloft.net +13 -0
#   [SPARC64]: compat select and futex need %o4 zero-extended.
# 
diff -Nru a/arch/sparc64/kernel/sys32.S b/arch/sparc64/kernel/sys32.S
--- a/arch/sparc64/kernel/sys32.S	2004-05-29 12:28:12 -07:00
+++ b/arch/sparc64/kernel/sys32.S	2004-05-29 12:28:12 -07:00
@@ -6,6 +6,7 @@
  * Copyright (C) 1998 Jakub Jelinek   (jj@ultra.linux.cz)
  */
 
+#include <linux/config.h>
 #include <asm/errno.h>
 
 /* NOTE: call as jump breaks return stack, we have to avoid that */
@@ -77,6 +78,18 @@
 sys32_mq_timedreceive:
 	sethi		%hi(compat_sys_mq_timedreceive), %g1
 	jmpl		%g1 + %lo(compat_sys_mq_timedreceive), %g0
+	 srl		%o4, 0, %o4
+
+	.globl		sys32_select
+sys32_select:
+	sethi		%hi(compat_sys_select), %g1
+	jmpl		%g1 + %lo(compat_sys_select), %g0
+	 srl		%o4, 0, %o4
+
+	.globl		sys32_futex
+sys32_futex:
+	sethi		%hi(compat_sys_futex), %g1
+	jmpl		%g1 + %lo(compat_sys_futex), %g0
 	 srl		%o4, 0, %o4
 
 	.align		32
diff -Nru a/arch/sparc64/kernel/systbls.S b/arch/sparc64/kernel/systbls.S
--- a/arch/sparc64/kernel/systbls.S	2004-05-29 12:28:12 -07:00
+++ b/arch/sparc64/kernel/systbls.S	2004-05-29 12:28:12 -07:00
@@ -37,7 +37,7 @@
 	.word sys_madvise, sys_vhangup, sys32_truncate64, sys_mincore, sys32_getgroups16
 /*80*/	.word sys32_setgroups16, sys_getpgrp, sys_setgroups, compat_sys_setitimer, sys32_ftruncate64
 	.word sys_swapon, compat_sys_getitimer, sys_setuid, sys_sethostname, sys_setgid
-/*90*/	.word sys_dup2, sys_setfsuid, compat_sys_fcntl, compat_sys_select, sys_setfsgid
+/*90*/	.word sys_dup2, sys_setfsuid, compat_sys_fcntl, sys32_select, sys_setfsgid
 	.word sys_fsync, sys_setpriority32, sys_nis_syscall, sys_nis_syscall, sys_nis_syscall
 /*100*/ .word sys_getpriority, sys32_rt_sigreturn, sys32_rt_sigaction, sys32_rt_sigprocmask, sys32_rt_sigpending
 	.word sys32_rt_sigtimedwait, sys32_rt_sigqueueinfo, sys32_rt_sigsuspend, sys_setresuid, sys_getresuid
@@ -47,7 +47,7 @@
 	.word sys_nis_syscall, sys32_setreuid16, sys32_setregid16, sys_rename, sys_truncate
 /*130*/	.word sys_ftruncate, sys_flock, sys_lstat64, sys_nis_syscall, sys_nis_syscall
 	.word sys_nis_syscall, sys_mkdir, sys_rmdir, sys32_utimes, sys_stat64
-/*140*/	.word sys32_sendfile64, sys_nis_syscall, compat_sys_futex, sys_gettid, compat_sys_getrlimit
+/*140*/	.word sys32_sendfile64, sys_nis_syscall, sys32_futex, sys_gettid, compat_sys_getrlimit
 	.word compat_sys_setrlimit, sys_pivot_root, sys32_prctl, sys32_pciconfig_read, sys32_pciconfig_write
 /*150*/	.word sys_nis_syscall, sys_nis_syscall, sys_nis_syscall, sys_poll, sys_getdents64
 	.word compat_sys_fcntl64, sys_ni_syscall, compat_sys_statfs, compat_sys_fstatfs, sys_oldumount
@@ -65,7 +65,7 @@
 	.word sys32_ipc, sys32_sigreturn, sys_clone, sys_nis_syscall, sys32_adjtimex
 /*220*/	.word compat_sys_sigprocmask, sys_ni_syscall, sys32_delete_module, sys_ni_syscall, sys_getpgid
 	.word sys32_bdflush, sys32_sysfs, sys_nis_syscall, sys32_setfsuid16, sys32_setfsgid16
-/*230*/	.word compat_sys_select, sys_time, sys_nis_syscall, sys_stime, compat_statfs64
+/*230*/	.word sys32_select, sys_time, sys_nis_syscall, sys_stime, compat_statfs64
 	.word compat_fstatfs64, sys_llseek, sys_mlock, sys_munlock, sys_mlockall
 /*240*/	.word sys_munlockall, sys_sched_setparam, sys_sched_getparam, sys_sched_setscheduler, sys_sched_getscheduler
 	.word sys_sched_yield, sys_sched_get_priority_max, sys_sched_get_priority_min, sys32_sched_rr_get_interval, compat_sys_nanosleep

Re: compat syscall args

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2423 bytes --]

Hi Dave,

On Sat, 29 May 2004 12:23:19 -0700 "David S. Miller" <davem@redhat.com> wrote:
>
> 
> Arnd asked:
> 
> > If sparc64 has this problem only for the fifth syscall argument, 
> > does that mean that e.g. compat_sys_futex and 
> > compat_sys_mq_timed{send,receive} have the same bug? If this is
> > a more general, i.e. not limited to the last argument, there is a
> > potential problem in lots of syscalls.
> 
> Here is the issue.  In the sparc64 C calling conventions, it is
> assumed that 32-bit signed values are sign extended by the
> caller.
> 
> This means that, at syscall invocation time, we have to choose
> between either:
> 
> 1) sign extending all syscall args for the C code, then explicitly
>    zero-extending all non-signed syscall args.  This would require
>    the most amount of compat layer code help.
> 
> 2) zero extending all syscall args for the C code, then expliticly
>    sign-extending all signed syscall args.
> 
> 3) some mixture of 1 and 2
> 
> #3 is what sparc64 does, it hits the highest number of system
> call arguments correctly.  Specifically we:
> 
> arg0: zero-extend
> arg1: zero-extend
> arg2: zero-extend
> arg3: zero-extend
> arg4: leave as-is
> arg5: leave as-is
> 
> I remember discussing this with Andi Kleen before.

Yeah, you and Andi and I (and others, I think) had this discussion, but it ended like this:

--------------------
Date: Thu, 13 Jun 2002 22:56:52 -0700 (PDT)
Message-Id: <20020613.225652.109741623.davem@redhat.com>
To: ak@suse.de
Cc: sfr@canb.auug.org.au, ralf@gnu.org, engebret@us.ibm.com,
   schwidefsky@de.ibm.com, linux390@de.ibm.com, anton@samba.org
Subject: Re: [PATCH] Consolidate sys32_utime
From: "David S. Miller" <davem@redhat.com>

   From: Andi Kleen <ak@suse.de>
   Date: Fri, 14 Jun 2002 07:53:10 +0200

   On Thu, Jun 13, 2002 at 10:37:23PM -0700, David S. Miller wrote:
   > We could easily change that to "all args zero-extended" if that makes
   > everything easier.  It probably does.
   
   I've had good experiences with zero extension at least so far.
   Would be probably good just for consistency.

So be it, the convention is that all arguments are zero extended from
32-bits to 64-bits when the syscall is invoked.
----------------------------------------------------------------------

Did something change along the way?
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: compat syscall args

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On Tuesday 01 June 2004 07:06, Stephen Rothwell wrote:
> On Sat, 29 May 2004 12:23:19 -0700 "David S. Miller" <davem@redhat.com> wrote:
> > I remember discussing this with Andi Kleen before.
> 
> Yeah, you and Andi and I (and others, I think) had this discussion, but it ended like this:
> 
> > Subject: Re: [PATCH] Consolidate sys32_utime
> > From: "David S. Miller" <davem@redhat.com>
> > 
> > So be it, the convention is that all arguments are zero extended from
> > 32-bits to 64-bits when the syscall is invoked.
> 
> Did something change along the way?

I wasn't aware of the convention but it absolutely makes sense. If I find
the time, I'll go through the existing compat_sys_* handlers to see if they
all do sign-extension correctly. Note that on s390, we also need 31-bit
zero-extension for pointers, which is done in architecture-specific code.
Otherwise every compat_* syscall would need to use compat_uptr_t arguments
which appears unnecessarily ugly to me.

Also, what should be the conversion for positive signed arguments like the
futex 'op' value? Sign-extension would be the formally correct solution,
but simply using the zero-extended value (like we do in most places) works
just as well.

	Arnd <><

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: compat syscall args

[David S. Miller]

On Tue, 1 Jun 2004 11:07:42 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> Also, what should be the conversion for positive signed arguments like the
> futex 'op' value? Sign-extension would be the formally correct solution,
> but simply using the zero-extended value (like we do in most places) works
> just as well.

Personally I think it makes more sense to do what sparc64 does
which is:

1) The syscall32 entry code extends each arg in a fixed way.
   Ie. arg0-3 are zero-extended, arg4-5 are sign-extended
   or whatever.  I posted the choices we use on sparc64 just
   the other day.

2) For each syscall where this default set of extensions is
   not correct, little assembler or C stubs are used to correct
   the extensions made by the default code.

It is the most optimal solution.  We only need 13 or so stubs
on sparc64 with the defaults we've choosen.

Re: compat syscall args

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

On Saturday 29 May 2004 21:31, David S. Miller wrote:
> As it turns out we're taking care of this already via stubs
> in arch/sparc64/kernel/sys32.S, I just need to add them for
> select and futex.

Ah, thanks for solving this mystery. 

select and futex may not be the only ones that need to be fixed
for sparc64. I grepped through the list of syscalls that you
call without a sys32.S wrapper and found these that take at least
five arguments:

compat_sys_io_getevents, compat_sys_mount, sys_fsetxattr,
sys_setsockopt, sys_llseek, sys_mremap, sys_remap_file_pages,
sys_setxattr, sys_lsetxattr, sys_fsetxattr, sys32_fadvise64,
sys32_fadvise64_64, sys32_ipc, sys32_mremap, sys32_pciconfig_read,
sys32_pciconfig_write, sys32_prctl, sys32_pread64, sys32_pwrite64,
sys32_rt_sigaction

I did not check which ones are safe anyway, but you probably
want to go through that list.

	Arnd <><

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: compat syscall args

[David S. Miller]

On Wed, 2 Jun 2004 03:04:18 +1000
Anton Blanchard <anton@samba.org> wrote:

> > It would be better to do this consistently over all architectures
> > and do the sign extension (which is much less common than zero
> > extension) always in C code. Then when someone adds a new compat
> > handler the chances are high that it will just work over multiple
> > architectures (ok minus s390) without much more changes. 
> 
> On ppc64 we now zero extend all arguments. I too would like to see the
> sign extension done in the common compat code, at the moment we have to
> be careful to do it before calling compat code.

Ok, I'm doing some auditing of the sparc64 compat syscall table
and all the args to the syscalls.  I'll take this thread further
after I'm done with that.

Sit tight.

Re: compat syscall args

[Anton Blanchard]

 
> It would be better to do this consistently over all architectures
> and do the sign extension (which is much less common than zero
> extension) always in C code. Then when someone adds a new compat
> handler the chances are high that it will just work over multiple
> architectures (ok minus s390) without much more changes. 

On ppc64 we now zero extend all arguments. I too would like to see the
sign extension done in the common compat code, at the moment we have to
be careful to do it before calling compat code.

Anton

Re: compat syscall args

[Andi Kleen]

"David S. Miller" <davem@redhat.com> writes:

> Personally I think it makes more sense to do what sparc64 does
> which is:
>
> 1) The syscall32 entry code extends each arg in a fixed way.
>    Ie. arg0-3 are zero-extended, arg4-5 are sign-extended
>    or whatever.  I posted the choices we use on sparc64 just
>    the other day.

I don't see where you find that many arguments that need
sign extension? iirc they are quite rare.


> 2) For each syscall where this default set of extensions is
>    not correct, little assembler or C stubs are used to correct
>    the extensions made by the default code.
>
> It is the most optimal solution.  We only need 13 or so stubs
> on sparc64 with the defaults we've choosen.

It would be better to do this consistently over all architectures
and do the sign extension (which is much less common than zero
extension) always in C code. Then when someone adds a new compat
handler the chances are high that it will just work over multiple
architectures (ok minus s390) without much more changes. 

Actually if someone demonstrated that doing sign extension in assembly
helps a lot then I would not be opposed to doing it on x86-64 
too just for consistency.

I would be actually not against doing the s390 compat_uptr() changes
in C too, although it wouldn't help them with the "one handler
for everybody" goal, since it can be only tested on s390.

-Andi