Re: Writable sys_call_table (was: Re: [uClinux-dev] [PATCH] m68k: Merge mmu and non-mmu versions of sys_call_table)

Re: Writable sys_call_table (was: Re: [uClinux-dev] [PATCH] m68k: Merge mmu and non-mmu versions of sys_call_table)

[Arnd Bergmann]

On Wednesday 13 April 2011, Geert Uytterhoeven wrote:
> On Thu, Apr 7, 2011 at 10:29, Andreas Schwab <schwab@linux-m68k.org> wrote:
> > Geert Uytterhoeven <geert@linux-m68k.org> writes:
> >> Isn't there a reason it was read-write on m68k, like the table may be changed
> >> at runtime (to install rootkits :-)? Have to check what the other arches do...
> >
> > Initially the syscall_table in Linux has always been writable, bb152f53
> > ("x86/x86_64: mark rodata section read-only: make some datastructures
> > const") made it read-only on x86.  Apparently nobody bothered to do the
> > equivalent change on m68k (I don't think anything makes the kernel text
> > segment write protected anyway).
> 
> 11 arches still store it in "data", including the 4 using the new
> asm-generic/unistd.h
> framework. 9 use "rodata" and 6 use "text".
> The constness of C "extern" declarations doesn't necessarily matches the
> actual sections.
> 

Thanks for pointing this out. Should we apply this patch?
---
[PATCH] mark sys_call_table as const

There is no reason to have sys_call_table writable, and putting
it into the rodata section can make it harder for malicious users
to overwrite the entry points.

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

diff --git a/arch/score/kernel/sys_call_table.c b/arch/score/kernel/sys_call_table.c
index 287369b..7be73dc 100644
--- a/arch/score/kernel/sys_call_table.c
+++ b/arch/score/kernel/sys_call_table.c
@@ -7,6 +7,6 @@
 #undef __SYSCALL
 #define __SYSCALL(nr, call) [nr] = (call),
 
-void *sys_call_table[__NR_syscalls] = {
+const void *sys_call_table[__NR_syscalls] = {
 #include <asm/unistd.h>
 };
diff --git a/arch/tile/kernel/sys.c b/arch/tile/kernel/sys.c
index e2187d2..3f2ba14 100644
--- a/arch/tile/kernel/sys.c
+++ b/arch/tile/kernel/sys.c
@@ -122,7 +122,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
  * Note that we can't include <linux/unistd.h> here since the header
  * guard will defeat us; <asm/unistd.h> checks for __SYSCALL as well.
  */
-void *sys_call_table[__NR_syscalls] = {
+const void *sys_call_table[__NR_syscalls] = {
 	[0 ... __NR_syscalls-1] = sys_ni_syscall,
 #include <asm/unistd.h>
 };
diff --git a/arch/tile/kernel/compat.c b/arch/tile/kernel/compat.c
index dbc213a..d221452 100644
--- a/arch/tile/kernel/compat.c
+++ b/arch/tile/kernel/compat.c
@@ -166,7 +166,7 @@ long tile_compat_sys_msgrcv(int msqid,
  * Note that we can't include <linux/unistd.h> here since the header
  * guard will defeat us; <asm/unistd.h> checks for __SYSCALL as well.
  */
-void *compat_sys_call_table[__NR_syscalls] = {
+const void *compat_sys_call_table[__NR_syscalls] = {
 	[0 ... __NR_syscalls-1] = sys_ni_syscall,
 #include <asm/unistd.h>
 };
diff --git a/arch/unicore32/kernel/sys.c b/arch/unicore32/kernel/sys.c
index 3afe60a..7a16c7e 100644
--- a/arch/unicore32/kernel/sys.c
+++ b/arch/unicore32/kernel/sys.c
@@ -120,7 +120,7 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
 #define __SYSCALL(nr, call)	[nr] = (call),
 
 /* Note that we don't include <linux/unistd.h> but <asm/unistd.h> */
-void *sys_call_table[__NR_syscalls] = {
+const void *sys_call_table[__NR_syscalls] = {
 	[0 ... __NR_syscalls-1] = sys_ni_syscall,
 #include <asm/unistd.h>
 };

Re: Writable sys_call_table (was: Re: [uClinux-dev] [PATCH] m68k: Merge mmu and non-mmu versions of sys_call_table)

[Andreas Schwab]

Arnd Bergmann <arnd@arndb.de> writes:

> diff --git a/arch/score/kernel/sys_call_table.c b/arch/score/kernel/sys_call_table.c
> index 287369b..7be73dc 100644
> --- a/arch/score/kernel/sys_call_table.c
> +++ b/arch/score/kernel/sys_call_table.c
> @@ -7,6 +7,6 @@
>  #undef __SYSCALL
>  #define __SYSCALL(nr, call) [nr] = (call),
>  
> -void *sys_call_table[__NR_syscalls] = {
> +const void *sys_call_table[__NR_syscalls] = {

That's not making it read-only.  You need to move the const to the other
side of the pointer.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Writable sys_call_table (was: Re: [uClinux-dev] [PATCH] m68k: Merge mmu and non-mmu versions of sys_call_table)

[Arnd Bergmann]

On Monday 18 April 2011, Andreas Schwab wrote:
> >  
> > -void *sys_call_table[__NR_syscalls] = {
> > +const void *sys_call_table[__NR_syscalls] = {
> 
> That's not making it read-only.  You need to move the const to the other
> side of the pointer.

D'oh!

8<--------
[PATCH] mark sys_call_table as const

There is no reason to have sys_call_table writable, and putting
it into the rodata section can make it harder for malicious users
to overwrite the entry points.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
diff --git a/arch/score/kernel/sys_call_table.c b/arch/score/kernel/sys_call_table.c
index 287369b..6d61120 100644
--- a/arch/score/kernel/sys_call_table.c
+++ b/arch/score/kernel/sys_call_table.c
@@ -7,6 +7,6 @@
 #undef __SYSCALL
 #define __SYSCALL(nr, call) [nr] = (call),
 
-void *sys_call_table[__NR_syscalls] = {
+void *const sys_call_table[__NR_syscalls] = {
 #include <asm/unistd.h>
 };
diff --git a/arch/tile/include/asm/syscalls.h b/arch/tile/include/asm/syscalls.h
index 3b5507c..c0d6914 100644
--- a/arch/tile/include/asm/syscalls.h
+++ b/arch/tile/include/asm/syscalls.h
@@ -25,9 +25,9 @@
 #include <linux/compat.h>
 
 /* The array of function pointers for syscalls. */
-extern void *sys_call_table[];
+extern void *const sys_call_table[];
 #ifdef CONFIG_COMPAT
-extern void *compat_sys_call_table[];
+extern void *const compat_sys_call_table[];
 #endif
 
 /*
diff --git a/arch/tile/kernel/compat.c b/arch/tile/kernel/compat.c
index dbc213a..aedf769 100644
--- a/arch/tile/kernel/compat.c
+++ b/arch/tile/kernel/compat.c
@@ -166,7 +166,7 @@ long tile_compat_sys_msgrcv(int msqid,
  * Note that we can't include <linux/unistd.h> here since the header
  * guard will defeat us; <asm/unistd.h> checks for __SYSCALL as well.
  */
-void *compat_sys_call_table[__NR_syscalls] = {
+void *const compat_sys_call_table[__NR_syscalls] = {
 	[0 ... __NR_syscalls-1] = sys_ni_syscall,
 #include <asm/unistd.h>
 };
diff --git a/arch/tile/kernel/sys.c b/arch/tile/kernel/sys.c
index e2187d2..1fb2480 100644
--- a/arch/tile/kernel/sys.c
+++ b/arch/tile/kernel/sys.c
@@ -122,7 +122,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
  * Note that we can't include <linux/unistd.h> here since the header
  * guard will defeat us; <asm/unistd.h> checks for __SYSCALL as well.
  */
-void *sys_call_table[__NR_syscalls] = {
+void *const sys_call_table[__NR_syscalls] = {
 	[0 ... __NR_syscalls-1] = sys_ni_syscall,
 #include <asm/unistd.h>
 };
diff --git a/arch/unicore32/kernel/sys.c b/arch/unicore32/kernel/sys.c
index 3afe60a..00f3046 100644
--- a/arch/unicore32/kernel/sys.c
+++ b/arch/unicore32/kernel/sys.c
@@ -120,7 +120,7 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
 #define __SYSCALL(nr, call)	[nr] = (call),
 
 /* Note that we don't include <linux/unistd.h> but <asm/unistd.h> */
-void *sys_call_table[__NR_syscalls] = {
+void *const sys_call_table[__NR_syscalls] = {
 	[0 ... __NR_syscalls-1] = sys_ni_syscall,
 #include <asm/unistd.h>
 };

Re: Writable sys_call_table (was: Re: [uClinux-dev] [PATCH] m68k: Merge mmu and non-mmu versions of sys_call_table)

[Finn Thain]

On Tue, 19 Apr 2011, Arnd Bergmann wrote:

> On Monday 18 April 2011, Andreas Schwab wrote:
> > >  
> > > -void *sys_call_table[__NR_syscalls] = {
> > > +const void *sys_call_table[__NR_syscalls] = {
> > 
> > That's not making it read-only.  You need to move the const to the other
> > side of the pointer.
> 
> D'oh!
> 
> 8<--------
> [PATCH] mark sys_call_table as const
> 
> There is no reason to have sys_call_table writable, and putting
> it into the rodata section can make it harder for malicious users
> to overwrite the entry points.

Wouldn't that require const void * const sys_call_table[] ?

Finn

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> diff --git a/arch/score/kernel/sys_call_table.c b/arch/score/kernel/sys_call_table.c
> index 287369b..6d61120 100644
> --- a/arch/score/kernel/sys_call_table.c
> +++ b/arch/score/kernel/sys_call_table.c
> @@ -7,6 +7,6 @@
>  #undef __SYSCALL
>  #define __SYSCALL(nr, call) [nr] = (call),
>  
> -void *sys_call_table[__NR_syscalls] = {
> +void *const sys_call_table[__NR_syscalls] = {
>  #include <asm/unistd.h>
>  };
> diff --git a/arch/tile/include/asm/syscalls.h b/arch/tile/include/asm/syscalls.h
> index 3b5507c..c0d6914 100644
> --- a/arch/tile/include/asm/syscalls.h
> +++ b/arch/tile/include/asm/syscalls.h
> @@ -25,9 +25,9 @@
>  #include <linux/compat.h>
>  
>  /* The array of function pointers for syscalls. */
> -extern void *sys_call_table[];
> +extern void *const sys_call_table[];
>  #ifdef CONFIG_COMPAT
> -extern void *compat_sys_call_table[];
> +extern void *const compat_sys_call_table[];
>  #endif
>  
>  /*
> diff --git a/arch/tile/kernel/compat.c b/arch/tile/kernel/compat.c
> index dbc213a..aedf769 100644
> --- a/arch/tile/kernel/compat.c
> +++ b/arch/tile/kernel/compat.c
> @@ -166,7 +166,7 @@ long tile_compat_sys_msgrcv(int msqid,
>   * Note that we can't include <linux/unistd.h> here since the header
>   * guard will defeat us; <asm/unistd.h> checks for __SYSCALL as well.
>   */
> -void *compat_sys_call_table[__NR_syscalls] = {
> +void *const compat_sys_call_table[__NR_syscalls] = {
>  	[0 ... __NR_syscalls-1] = sys_ni_syscall,
>  #include <asm/unistd.h>
>  };
> diff --git a/arch/tile/kernel/sys.c b/arch/tile/kernel/sys.c
> index e2187d2..1fb2480 100644
> --- a/arch/tile/kernel/sys.c
> +++ b/arch/tile/kernel/sys.c
> @@ -122,7 +122,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
>   * Note that we can't include <linux/unistd.h> here since the header
>   * guard will defeat us; <asm/unistd.h> checks for __SYSCALL as well.
>   */
> -void *sys_call_table[__NR_syscalls] = {
> +void *const sys_call_table[__NR_syscalls] = {
>  	[0 ... __NR_syscalls-1] = sys_ni_syscall,
>  #include <asm/unistd.h>
>  };
> diff --git a/arch/unicore32/kernel/sys.c b/arch/unicore32/kernel/sys.c
> index 3afe60a..00f3046 100644
> --- a/arch/unicore32/kernel/sys.c
> +++ b/arch/unicore32/kernel/sys.c
> @@ -120,7 +120,7 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
>  #define __SYSCALL(nr, call)	[nr] = (call),
>  
>  /* Note that we don't include <linux/unistd.h> but <asm/unistd.h> */
> -void *sys_call_table[__NR_syscalls] = {
> +void *const sys_call_table[__NR_syscalls] = {
>  	[0 ... __NR_syscalls-1] = sys_ni_syscall,
>  #include <asm/unistd.h>
>  };
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Writable sys_call_table (was: Re: [uClinux-dev] [PATCH] m68k: Merge mmu and non-mmu versions of sys_call_table)

[Andreas Schwab]

Finn Thain <fthain@telegraphics.com.au> writes:

> Wouldn't that require const void * const sys_call_table[] ?

No, the right signature would be void (*const sys_call_table[])(void)
since this is an array of function pointers.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Writable sys_call_table (was: Re: [uClinux-dev] [PATCH] m68k: Merge mmu and non-mmu versions of sys_call_table)

[Arnd Bergmann]

On Tuesday 19 April 2011, Andreas Schwab wrote:
> Finn Thain <fthain@telegraphics.com.au> writes:
> 
> > Wouldn't that require const void * const sys_call_table[] ?
> 
> No, the right signature would be void (*const sys_call_table[])(void)
> since this is an array of function pointers.
> 

But it's not an array of pointers to function with signature
void f(void). It's an array of void pointers that we assign
to function pointers of various types.

	Arnd

Re: Writable sys_call_table (was: Re: [uClinux-dev] [PATCH] m68k: Merge mmu and non-mmu versions of sys_call_table)

[Andreas Schwab]

Arnd Bergmann <arnd@arndb.de> writes:

> But it's not an array of pointers to function with signature
> void f(void).

It does not matter which kind of function pointer you use since all
function pointers are alike (unlike data pointers).

> It's an array of void pointers that we assign to function pointers of
> various types.

It is an array of function pointers.  It is only void * because that
suppresses a warning (it is only accessed through assembly, so any type
the size of a pointer would do).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Writable sys_call_table (was: Re: [uClinux-dev] [PATCH] m68k: Merge mmu and non-mmu versions of sys_call_table)

[Finn Thain]

On Tue, 19 Apr 2011, Andreas Schwab wrote:

> Finn Thain <fthain@telegraphics.com.au> writes:
> 
> > Wouldn't that require const void * const sys_call_table[] ?
> 
> No, the right signature would be void (*const sys_call_table[])(void)
> since this is an array of function pointers.

Right. I mis-parsed the definition as a pointer to an array... Sorry about 
the noise.

Finn


> 
> Andreas.
> 
> 

Writable sys_call_table (was: Re: [uClinux-dev] [PATCH] m68k: Merge mmu and non-mmu versions of sys_call_table)

[Geert Uytterhoeven]

On Thu, Apr 7, 2011 at 10:29, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> Isn't there a reason it was read-write on m68k, like the table may be changed
>> at runtime (to install rootkits :-)? Have to check what the other arches do...
>
> Initially the syscall_table in Linux has always been writable, bb152f53
> ("x86/x86_64: mark rodata section read-only: make some datastructures
> const") made it read-only on x86.  Apparently nobody bothered to do the
> equivalent change on m68k (I don't think anything makes the kernel text
> segment write protected anyway).

11 arches still store it in "data", including the 4 using the new
asm-generic/unistd.h
framework. 9 use "rodata" and 6 use "text".
The constness of C "extern" declarations doesn't necessarily matches the
actual sections.

alpha:          .data
arm:            presumably .text?
avr32:          .section .rodata,"a",@progbits
blackfin:       .section .l1.data / .data
cris:           .section .rodata,"a"
frv:            .section .rodata
h8300:          .section .text
ia64:           .rodata
                extern unsigned long sys_call_table[NR_syscalls];
m32r:           .section .rodata,"a"
m68k:           .data (mmu), .text (nommu)
m68knommu:      .text
microblaze:     .section .rodata,"a"
mips:           presumably .text?
mn10300:        .data
                extern const unsigned long sys_call_table[];
parisc:         .section .rodata,"a"
powerpc:        presumably .text?
                extern unsigned long *sys_call_table;
                static void *spu_syscall_table[] (SPU in CBEA)
s390:           .section .rodata, "a"
                extern const unsigned int sys_call_table[];
score:          void *sys_call_table[__NR_syscalls[] = { ... }
sh:             .data
                extern const unsigned long sys_call_table[];
sh64:           .section .data, "aw"
sparc:          .data
                extern const unsigned int sys_call_table[];
sparc64:        .text
tile:           void *sys_call_table[__NR_syscalls] = { ... }
                void *compat_sys_call_table[__NR_syscalls] = { ... }
um:             extern syscall_handler_t *sys_call_table[];
unicore32:      void *sys_call_table[__NR_syscalls] = { ... }
x86:            .section .rodata,"a"
                extern const unsigned long sys_call_table[];
                const sys_call_ptr_t
sys_call_table[__NR_syscall_max+1] = { ...  }
xtensa:         syscall_t sys_call_table[__NR_syscall_count] = { ... }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds