[v9,2/4] seccomp: switch system call argument type to void *

Submitted by Tycho Andersen on Dec. 3, 2018, 3:28 a.m.

Details

Message ID 20181203032827.27978-3-tycho@tycho.ws
State New
Series "seccomp trap to userspace"
Headers show

Commit Message

Tycho Andersen Dec. 3, 2018, 3:28 a.m.
The const qualifier causes problems for any code that wants to write to the
third argument of the seccomp syscall, as we will do in a future patch in
this series.

The third argument to the seccomp syscall is documented as void *, so
rather than just dropping the const, let's switch everything to use void *
as well.

I believe this is safe because of 1. the documentation above, 2. there's no
real type information exported about syscalls anywhere besides the man
pages.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: Kees Cook <keescook@chromium.org>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Eric W. Biederman <ebiederm@xmission.com>
CC: "Serge E. Hallyn" <serge@hallyn.com>
CC: Christian Brauner <christian@brauner.io>
CC: Tyler Hicks <tyhicks@canonical.com>
CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
---
 include/linux/seccomp.h | 2 +-
 kernel/seccomp.c        | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index e5320f6c8654..b5103c019cf4 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -43,7 +43,7 @@  extern void secure_computing_strict(int this_syscall);
 #endif
 
 extern long prctl_get_seccomp(void);
-extern long prctl_set_seccomp(unsigned long, char __user *);
+extern long prctl_set_seccomp(unsigned long, void __user *);
 
 static inline int seccomp_mode(struct seccomp *s)
 {
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 96afc32e041d..393e029f778a 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -924,7 +924,7 @@  static long seccomp_get_action_avail(const char __user *uaction)
 
 /* Common entry point for both prctl and syscall. */
 static long do_seccomp(unsigned int op, unsigned int flags,
-		       const char __user *uargs)
+		       void __user *uargs)
 {
 	switch (op) {
 	case SECCOMP_SET_MODE_STRICT:
@@ -944,7 +944,7 @@  static long do_seccomp(unsigned int op, unsigned int flags,
 }
 
 SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
-			 const char __user *, uargs)
+			 void __user *, uargs)
 {
 	return do_seccomp(op, flags, uargs);
 }
@@ -956,10 +956,10 @@  SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
  *
  * Returns 0 on success or -EINVAL on failure.
  */
-long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
+long prctl_set_seccomp(unsigned long seccomp_mode, void __user *filter)
 {
 	unsigned int op;
-	char __user *uargs;
+	void __user *uargs;
 
 	switch (seccomp_mode) {
 	case SECCOMP_MODE_STRICT:

Comments

Serge E. Hallyn Dec. 3, 2018, 5:01 a.m.
On Sun, Dec 02, 2018 at 08:28:25PM -0700, Tycho Andersen wrote:
> The const qualifier causes problems for any code that wants to write to the
> third argument of the seccomp syscall, as we will do in a future patch in
> this series.
> 
> The third argument to the seccomp syscall is documented as void *, so
> rather than just dropping the const, let's switch everything to use void *
> as well.
> 
> I believe this is safe because of 1. the documentation above, 2. there's no
> real type information exported about syscalls anywhere besides the man
> pages.
> 
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> CC: Kees Cook <keescook@chromium.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Eric W. Biederman <ebiederm@xmission.com>
> CC: "Serge E. Hallyn" <serge@hallyn.com>

Acked-by: Serge Hallyn <serge@hallyn.com>

Though I'm not entirely convinced there will be no ill effects of changing
the argument type.  I'll feel comfortable when Michael and Paul say it's
fine :)

> CC: Christian Brauner <christian@brauner.io>
> CC: Tyler Hicks <tyhicks@canonical.com>
> CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> ---
>  include/linux/seccomp.h | 2 +-
>  kernel/seccomp.c        | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index e5320f6c8654..b5103c019cf4 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -43,7 +43,7 @@ extern void secure_computing_strict(int this_syscall);
>  #endif
>  
>  extern long prctl_get_seccomp(void);
> -extern long prctl_set_seccomp(unsigned long, char __user *);
> +extern long prctl_set_seccomp(unsigned long, void __user *);
>  
>  static inline int seccomp_mode(struct seccomp *s)
>  {
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 96afc32e041d..393e029f778a 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -924,7 +924,7 @@ static long seccomp_get_action_avail(const char __user *uaction)
>  
>  /* Common entry point for both prctl and syscall. */
>  static long do_seccomp(unsigned int op, unsigned int flags,
> -		       const char __user *uargs)
> +		       void __user *uargs)
>  {
>  	switch (op) {
>  	case SECCOMP_SET_MODE_STRICT:
> @@ -944,7 +944,7 @@ static long do_seccomp(unsigned int op, unsigned int flags,
>  }
>  
>  SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
> -			 const char __user *, uargs)
> +			 void __user *, uargs)
>  {
>  	return do_seccomp(op, flags, uargs);
>  }
> @@ -956,10 +956,10 @@ SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
>   *
>   * Returns 0 on success or -EINVAL on failure.
>   */
> -long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
> +long prctl_set_seccomp(unsigned long seccomp_mode, void __user *filter)
>  {
>  	unsigned int op;
> -	char __user *uargs;
> +	void __user *uargs;
>  
>  	switch (seccomp_mode) {
>  	case SECCOMP_MODE_STRICT:
> -- 
> 2.19.1
Paul Moore Dec. 4, 2018, 12:03 a.m.
On Mon, Dec 3, 2018 at 12:01 AM Serge E. Hallyn <serge@hallyn.com> wrote:
> On Sun, Dec 02, 2018 at 08:28:25PM -0700, Tycho Andersen wrote:
> > The const qualifier causes problems for any code that wants to write to the
> > third argument of the seccomp syscall, as we will do in a future patch in
> > this series.
> >
> > The third argument to the seccomp syscall is documented as void *, so
> > rather than just dropping the const, let's switch everything to use void *
> > as well.
> >
> > I believe this is safe because of 1. the documentation above, 2. there's no
> > real type information exported about syscalls anywhere besides the man
> > pages.
> >
> > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> > CC: Kees Cook <keescook@chromium.org>
> > CC: Andy Lutomirski <luto@amacapital.net>
> > CC: Oleg Nesterov <oleg@redhat.com>
> > CC: Eric W. Biederman <ebiederm@xmission.com>
> > CC: "Serge E. Hallyn" <serge@hallyn.com>
>
> Acked-by: Serge Hallyn <serge@hallyn.com>
>
> Though I'm not entirely convinced there will be no ill effects of changing
> the argument type.  I'll feel comfortable when Michael and Paul say it's
> fine :)

Well, looking at the seccomp(2) manpage on my system (dated
2018-02-02) the third argument is already shown as a "void *args":

 SYNOPSIS
      #include <linux/seccomp.h>
      #include <linux/filter.h>
      #include <linux/audit.h>
      #include <linux/signal.h>
      #include <sys/ptrace.h>

      int seccomp(unsigned int operation, unsigned int flags, void *args);

... so I think we're safe :)

From a libseccomp perspective, we always call seccomp(2) via
syscall(2) so it is unlikely we would ever run into problems, not too
mention that we are just talking about the pointer type used in the
kernel; from a syscall ABI perspective it is still a pointer value and
that is the important part.

> > CC: Christian Brauner <christian@brauner.io>
> > CC: Tyler Hicks <tyhicks@canonical.com>
> > CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
> > ---
> >  include/linux/seccomp.h | 2 +-
> >  kernel/seccomp.c        | 8 ++++----
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> > index e5320f6c8654..b5103c019cf4 100644
> > --- a/include/linux/seccomp.h
> > +++ b/include/linux/seccomp.h
> > @@ -43,7 +43,7 @@ extern void secure_computing_strict(int this_syscall);
> >  #endif
> >
> >  extern long prctl_get_seccomp(void);
> > -extern long prctl_set_seccomp(unsigned long, char __user *);
> > +extern long prctl_set_seccomp(unsigned long, void __user *);
> >
> >  static inline int seccomp_mode(struct seccomp *s)
> >  {
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 96afc32e041d..393e029f778a 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -924,7 +924,7 @@ static long seccomp_get_action_avail(const char __user *uaction)
> >
> >  /* Common entry point for both prctl and syscall. */
> >  static long do_seccomp(unsigned int op, unsigned int flags,
> > -                    const char __user *uargs)
> > +                    void __user *uargs)
> >  {
> >       switch (op) {
> >       case SECCOMP_SET_MODE_STRICT:
> > @@ -944,7 +944,7 @@ static long do_seccomp(unsigned int op, unsigned int flags,
> >  }
> >
> >  SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
> > -                      const char __user *, uargs)
> > +                      void __user *, uargs)
> >  {
> >       return do_seccomp(op, flags, uargs);
> >  }
> > @@ -956,10 +956,10 @@ SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
> >   *
> >   * Returns 0 on success or -EINVAL on failure.
> >   */
> > -long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
> > +long prctl_set_seccomp(unsigned long seccomp_mode, void __user *filter)
> >  {
> >       unsigned int op;
> > -     char __user *uargs;
> > +     void __user *uargs;
> >
> >       switch (seccomp_mode) {
> >       case SECCOMP_MODE_STRICT:
> > --
> > 2.19.1
kbuild test robot Dec. 4, 2018, 2:07 a.m.
Hi Tycho,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc5 next-20181203]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tycho-Andersen/seccomp-hoist-struct-seccomp_data-recalculation-higher/20181204-013450
config: i386-randconfig-x005-201848 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from kernel/seccomp.c:28:0:
>> include/linux/syscalls.h:239:18: error: conflicting types for 'sys_seccomp'
     asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
                     ^
   include/linux/syscalls.h:225:2: note: in expansion of macro '__SYSCALL_DEFINEx'
     __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
     ^~~~~~~~~~~~~~~~~
   include/linux/syscalls.h:216:36: note: in expansion of macro 'SYSCALL_DEFINEx'
    #define SYSCALL_DEFINE3(name, ...) SYSCALL_DEFINEx(3, _##name, __VA_ARGS__)
                                       ^~~~~~~~~~~~~~~
   kernel/seccomp.c:946:1: note: in expansion of macro 'SYSCALL_DEFINE3'
    SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
    ^~~~~~~~~~~~~~~
   In file included from kernel/seccomp.c:28:0:
   include/linux/syscalls.h:881:17: note: previous declaration of 'sys_seccomp' was here
    asmlinkage long sys_seccomp(unsigned int op, unsigned int flags,
                    ^~~~~~~~~~~

vim +/sys_seccomp +239 include/linux/syscalls.h

1bd21c6c2 Dominik Brodowski   2018-04-05  228  
e145242ea Dominik Brodowski   2018-04-09  229  /*
e145242ea Dominik Brodowski   2018-04-09  230   * The asmlinkage stub is aliased to a function named __se_sys_*() which
e145242ea Dominik Brodowski   2018-04-09  231   * sign-extends 32-bit ints to longs whenever needed. The actual work is
e145242ea Dominik Brodowski   2018-04-09  232   * done within __do_sys_*().
e145242ea Dominik Brodowski   2018-04-09  233   */
1bd21c6c2 Dominik Brodowski   2018-04-05  234  #ifndef __SYSCALL_DEFINEx
bed1ffca0 Frederic Weisbecker 2009-03-13  235  #define __SYSCALL_DEFINEx(x, name, ...)					\
bee200317 Arnd Bergmann       2018-06-19  236  	__diag_push();							\
bee200317 Arnd Bergmann       2018-06-19  237  	__diag_ignore(GCC, 8, "-Wattribute-alias",			\
bee200317 Arnd Bergmann       2018-06-19  238  		      "Type aliasing is used to sanitize syscall arguments");\
83460ec8d Andi Kleen          2013-11-12 @239  	asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))	\
e145242ea Dominik Brodowski   2018-04-09  240  		__attribute__((alias(__stringify(__se_sys##name))));	\
c9a211951 Howard McLauchlan   2018-03-21  241  	ALLOW_ERROR_INJECTION(sys##name, ERRNO);			\
e145242ea Dominik Brodowski   2018-04-09  242  	static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__));\
e145242ea Dominik Brodowski   2018-04-09  243  	asmlinkage long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__));	\
e145242ea Dominik Brodowski   2018-04-09  244  	asmlinkage long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__))	\
1a94bc347 Heiko Carstens      2009-01-14  245  	{								\
e145242ea Dominik Brodowski   2018-04-09  246  		long ret = __do_sys##name(__MAP(x,__SC_CAST,__VA_ARGS__));\
07fe6e00f Al Viro             2013-01-21  247  		__MAP(x,__SC_TEST,__VA_ARGS__);				\
2cf096668 Al Viro             2013-01-21  248  		__PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__));	\
2cf096668 Al Viro             2013-01-21  249  		return ret;						\
1a94bc347 Heiko Carstens      2009-01-14  250  	}								\
bee200317 Arnd Bergmann       2018-06-19  251  	__diag_pop();							\
e145242ea Dominik Brodowski   2018-04-09  252  	static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
1bd21c6c2 Dominik Brodowski   2018-04-05  253  #endif /* __SYSCALL_DEFINEx */
1a94bc347 Heiko Carstens      2009-01-14  254  

:::::: The code at line 239 was first introduced by commit
:::::: 83460ec8dcac14142e7860a01fa59c267ac4657c syscalls.h: use gcc alias instead of assembler aliases for syscalls

:::::: TO: Andi Kleen <ak@linux.intel.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Tycho Andersen Dec. 4, 2018, 2:17 a.m.
On Tue, Dec 04, 2018 at 10:07:38AM +0800, kbuild test robot wrote:
> Hi Tycho,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.20-rc5 next-20181203]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Tycho-Andersen/seccomp-hoist-struct-seccomp_data-recalculation-higher/20181204-013450
> config: i386-randconfig-x005-201848 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from kernel/seccomp.c:28:0:
> >> include/linux/syscalls.h:239:18: error: conflicting types for 'sys_seccomp'
>      asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
>                      ^
>    include/linux/syscalls.h:225:2: note: in expansion of macro '__SYSCALL_DEFINEx'
>      __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
>      ^~~~~~~~~~~~~~~~~
>    include/linux/syscalls.h:216:36: note: in expansion of macro 'SYSCALL_DEFINEx'
>     #define SYSCALL_DEFINE3(name, ...) SYSCALL_DEFINEx(3, _##name, __VA_ARGS__)
>                                        ^~~~~~~~~~~~~~~
>    kernel/seccomp.c:946:1: note: in expansion of macro 'SYSCALL_DEFINE3'
>     SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
>     ^~~~~~~~~~~~~~~
>    In file included from kernel/seccomp.c:28:0:
>    include/linux/syscalls.h:881:17: note: previous declaration of 'sys_seccomp' was here
>     asmlinkage long sys_seccomp(unsigned int op, unsigned int flags,
>                     ^~~~~~~~~~~

Huh, I have no idea why I don't see this, but even with the attached
config it still doesn't cause a problem for me. Anyway, I'll fix it up
and do some more investigating...

Tycho
Tycho Andersen Dec. 4, 2018, 2:34 a.m.
On Mon, Dec 03, 2018 at 07:17:26PM -0700, Tycho Andersen wrote:
> On Tue, Dec 04, 2018 at 10:07:38AM +0800, kbuild test robot wrote:
> > Hi Tycho,
> > 
> > I love your patch! Yet something to improve:
> > 
> > [auto build test ERROR on linus/master]
> > [also build test ERROR on v4.20-rc5 next-20181203]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Tycho-Andersen/seccomp-hoist-struct-seccomp_data-recalculation-higher/20181204-013450
> > config: i386-randconfig-x005-201848 (attached as .config)
> > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> > reproduce:
> >         # save the attached .config to linux build tree
> >         make ARCH=i386 
> > 
> > All errors (new ones prefixed by >>):
> > 
> >    In file included from kernel/seccomp.c:28:0:
> > >> include/linux/syscalls.h:239:18: error: conflicting types for 'sys_seccomp'
> >      asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
> >                      ^
> >    include/linux/syscalls.h:225:2: note: in expansion of macro '__SYSCALL_DEFINEx'
> >      __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
> >      ^~~~~~~~~~~~~~~~~
> >    include/linux/syscalls.h:216:36: note: in expansion of macro 'SYSCALL_DEFINEx'
> >     #define SYSCALL_DEFINE3(name, ...) SYSCALL_DEFINEx(3, _##name, __VA_ARGS__)
> >                                        ^~~~~~~~~~~~~~~
> >    kernel/seccomp.c:946:1: note: in expansion of macro 'SYSCALL_DEFINE3'
> >     SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
> >     ^~~~~~~~~~~~~~~
> >    In file included from kernel/seccomp.c:28:0:
> >    include/linux/syscalls.h:881:17: note: previous declaration of 'sys_seccomp' was here
> >     asmlinkage long sys_seccomp(unsigned int op, unsigned int flags,
> >                     ^~~~~~~~~~~
> 
> Huh, I have no idea why I don't see this, but even with the attached
> config it still doesn't cause a problem for me. Anyway, I'll fix it up
> and do some more investigating...

Oh, because it's "make ARCH=i386". Whoosh :)

Anyway, it's fixed for v10.

Tycho