diff mbox series

kernel/sysctl.c: Fix out-of-bounds access when setting file-max

Message ID 20190403153409.17307-1-will.deacon@arm.com
State Accepted
Commit 9002b21465fa4d829edfc94a5a441005cffaa972
Headers show
Series kernel/sysctl.c: Fix out-of-bounds access when setting file-max | expand

Commit Message

Will Deacon April 3, 2019, 3:34 p.m. UTC 
Commit 32a5ad9c2285 ("sysctl: handle overflow for file-max") hooked
up min/max values for the file-max sysctl parameter via the .extra1
and .extra2 fields in the corresponding struct ctl_table entry.

Unfortunately, the minimum value points at the global 'zero' variable,
which is an int. This results in a KASAN splat when accessed as a long
by proc_doulongvec_minmax on 64-bit architectures:

  | BUG: KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax+0x5d8/0x6a0
  | Read of size 8 at addr ffff2000133d1c20 by task systemd/1
  |
  | CPU: 0 PID: 1 Comm: systemd Not tainted 5.1.0-rc3-00012-g40b114779944 #2
  | Hardware name: linux,dummy-virt (DT)
  | Call trace:
  |  dump_backtrace+0x0/0x228
  |  show_stack+0x14/0x20
  |  dump_stack+0xe8/0x124
  |  print_address_description+0x60/0x258
  |  kasan_report+0x140/0x1a0
  |  __asan_report_load8_noabort+0x18/0x20
  |  __do_proc_doulongvec_minmax+0x5d8/0x6a0
  |  proc_doulongvec_minmax+0x4c/0x78
  |  proc_sys_call_handler.isra.19+0x144/0x1d8
  |  proc_sys_write+0x34/0x58
  |  __vfs_write+0x54/0xe8
  |  vfs_write+0x124/0x3c0
  |  ksys_write+0xbc/0x168
  |  __arm64_sys_write+0x68/0x98
  |  el0_svc_common+0x100/0x258
  |  el0_svc_handler+0x48/0xc0
  |  el0_svc+0x8/0xc
  |
  | The buggy address belongs to the variable:
  |  zero+0x0/0x40
  |
  | Memory state around the buggy address:
  |  ffff2000133d1b00: 00 00 00 00 00 00 00 00 fa fa fa fa 04 fa fa fa
  |  ffff2000133d1b80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa
  | >ffff2000133d1c00: fa fa fa fa 04 fa fa fa fa fa fa fa 00 00 00 00
  |                                ^
  |  ffff2000133d1c80: fa fa fa fa 00 fa fa fa fa fa fa fa 00 00 00 00
  |  ffff2000133d1d00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Fix the splat by introducing a unsigned long 'zero_ul' and using that
instead.

Fixes: 32a5ad9c2285 ("sysctl: handle overflow for file-max")
Cc: Christian Brauner <christian@brauner.io>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>

---
 kernel/sysctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.11.0

Comments

Christian Brauner April 3, 2019, 3:40 p.m. UTC | #1 
On Wed, Apr 03, 2019 at 04:34:09PM +0100, Will Deacon wrote:
> Commit 32a5ad9c2285 ("sysctl: handle overflow for file-max") hooked

> up min/max values for the file-max sysctl parameter via the .extra1

> and .extra2 fields in the corresponding struct ctl_table entry.

> 

> Unfortunately, the minimum value points at the global 'zero' variable,

> which is an int. This results in a KASAN splat when accessed as a long

> by proc_doulongvec_minmax on 64-bit architectures:

> 

>   | BUG: KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax+0x5d8/0x6a0

>   | Read of size 8 at addr ffff2000133d1c20 by task systemd/1

>   |

>   | CPU: 0 PID: 1 Comm: systemd Not tainted 5.1.0-rc3-00012-g40b114779944 #2

>   | Hardware name: linux,dummy-virt (DT)

>   | Call trace:

>   |  dump_backtrace+0x0/0x228

>   |  show_stack+0x14/0x20

>   |  dump_stack+0xe8/0x124

>   |  print_address_description+0x60/0x258

>   |  kasan_report+0x140/0x1a0

>   |  __asan_report_load8_noabort+0x18/0x20

>   |  __do_proc_doulongvec_minmax+0x5d8/0x6a0

>   |  proc_doulongvec_minmax+0x4c/0x78

>   |  proc_sys_call_handler.isra.19+0x144/0x1d8

>   |  proc_sys_write+0x34/0x58

>   |  __vfs_write+0x54/0xe8

>   |  vfs_write+0x124/0x3c0

>   |  ksys_write+0xbc/0x168

>   |  __arm64_sys_write+0x68/0x98

>   |  el0_svc_common+0x100/0x258

>   |  el0_svc_handler+0x48/0xc0

>   |  el0_svc+0x8/0xc

>   |

>   | The buggy address belongs to the variable:

>   |  zero+0x0/0x40

>   |

>   | Memory state around the buggy address:

>   |  ffff2000133d1b00: 00 00 00 00 00 00 00 00 fa fa fa fa 04 fa fa fa

>   |  ffff2000133d1b80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa

>   | >ffff2000133d1c00: fa fa fa fa 04 fa fa fa fa fa fa fa 00 00 00 00

>   |                                ^

>   |  ffff2000133d1c80: fa fa fa fa 00 fa fa fa fa fa fa fa 00 00 00 00

>   |  ffff2000133d1d00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

> 

> Fix the splat by introducing a unsigned long 'zero_ul' and using that

> instead.

> 

> Fixes: 32a5ad9c2285 ("sysctl: handle overflow for file-max")

> Cc: Christian Brauner <christian@brauner.io>

> Cc: Kees Cook <keescook@chromium.org>

> Cc: Andrew Morton <akpm@linux-foundation.org>

> Signed-off-by: Will Deacon <will.deacon@arm.com>


Hey Will, thanks! For the record, there's another patch by Matteo (Cced)
for the same thing:
https://lore.kernel.org/lkml/20190328130306.25384-1-mcroce@redhat.com/

He's proposing a slightly different version for the fix. I don't care
very much but I like that you've used the explicit unsigned long so:

Acked-by: Christian Brauner <christian@brauner.io>


Thanks to both of you for fixing this!

> ---

>  kernel/sysctl.c | 3 ++-

>  1 file changed, 2 insertions(+), 1 deletion(-)

> 

> diff --git a/kernel/sysctl.c b/kernel/sysctl.c

> index e5da394d1ca3..c9ec050bcf46 100644

> --- a/kernel/sysctl.c

> +++ b/kernel/sysctl.c

> @@ -128,6 +128,7 @@ static int zero;

>  static int __maybe_unused one = 1;

>  static int __maybe_unused two = 2;

>  static int __maybe_unused four = 4;

> +static unsigned long zero_ul;

>  static unsigned long one_ul = 1;

>  static unsigned long long_max = LONG_MAX;

>  static int one_hundred = 100;

> @@ -1750,7 +1751,7 @@ static struct ctl_table fs_table[] = {

>  		.maxlen		= sizeof(files_stat.max_files),

>  		.mode		= 0644,

>  		.proc_handler	= proc_doulongvec_minmax,

> -		.extra1		= &zero,

> +		.extra1		= &zero_ul,

>  		.extra2		= &long_max,

>  	},

>  	{

> -- 

> 2.11.0

>
Will Deacon April 3, 2019, 5:24 p.m. UTC | #2 
Hi Christian,

On Wed, Apr 03, 2019 at 05:40:45PM +0200, Christian Brauner wrote:
> On Wed, Apr 03, 2019 at 04:34:09PM +0100, Will Deacon wrote:

> > Commit 32a5ad9c2285 ("sysctl: handle overflow for file-max") hooked

> > up min/max values for the file-max sysctl parameter via the .extra1

> > and .extra2 fields in the corresponding struct ctl_table entry.

> > 

> > Unfortunately, the minimum value points at the global 'zero' variable,

> > which is an int. This results in a KASAN splat when accessed as a long

> > by proc_doulongvec_minmax on 64-bit architectures:

> > 

> >   | BUG: KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax+0x5d8/0x6a0

> >   | Read of size 8 at addr ffff2000133d1c20 by task systemd/1

> >   |

> >   | CPU: 0 PID: 1 Comm: systemd Not tainted 5.1.0-rc3-00012-g40b114779944 #2

> >   | Hardware name: linux,dummy-virt (DT)

> >   | Call trace:

> >   |  dump_backtrace+0x0/0x228

> >   |  show_stack+0x14/0x20

> >   |  dump_stack+0xe8/0x124

> >   |  print_address_description+0x60/0x258

> >   |  kasan_report+0x140/0x1a0

> >   |  __asan_report_load8_noabort+0x18/0x20

> >   |  __do_proc_doulongvec_minmax+0x5d8/0x6a0

> >   |  proc_doulongvec_minmax+0x4c/0x78

> >   |  proc_sys_call_handler.isra.19+0x144/0x1d8

> >   |  proc_sys_write+0x34/0x58

> >   |  __vfs_write+0x54/0xe8

> >   |  vfs_write+0x124/0x3c0

> >   |  ksys_write+0xbc/0x168

> >   |  __arm64_sys_write+0x68/0x98

> >   |  el0_svc_common+0x100/0x258

> >   |  el0_svc_handler+0x48/0xc0

> >   |  el0_svc+0x8/0xc

> >   |

> >   | The buggy address belongs to the variable:

> >   |  zero+0x0/0x40

> >   |

> >   | Memory state around the buggy address:

> >   |  ffff2000133d1b00: 00 00 00 00 00 00 00 00 fa fa fa fa 04 fa fa fa

> >   |  ffff2000133d1b80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa

> >   | >ffff2000133d1c00: fa fa fa fa 04 fa fa fa fa fa fa fa 00 00 00 00

> >   |                                ^

> >   |  ffff2000133d1c80: fa fa fa fa 00 fa fa fa fa fa fa fa 00 00 00 00

> >   |  ffff2000133d1d00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

> > 

> > Fix the splat by introducing a unsigned long 'zero_ul' and using that

> > instead.

> > 

> > Fixes: 32a5ad9c2285 ("sysctl: handle overflow for file-max")

> > Cc: Christian Brauner <christian@brauner.io>

> > Cc: Kees Cook <keescook@chromium.org>

> > Cc: Andrew Morton <akpm@linux-foundation.org>

> > Signed-off-by: Will Deacon <will.deacon@arm.com>

> 

> Hey Will, thanks! For the record, there's another patch by Matteo (Cced)

> for the same thing:

> https://lore.kernel.org/lkml/20190328130306.25384-1-mcroce@redhat.com/


Oops, sorry, I didn't spot that. I just ran into this while I was using
KASAN to investigate a different issue and quickly sent out a fix.

> He's proposing a slightly different version for the fix. I don't care

> very much but I like that you've used the explicit unsigned long so:

> 

> Acked-by: Christian Brauner <christian@brauner.io>

> 

> Thanks to both of you for fixing this!


I'm also not bothered about which patch gets in, as long as the problem is
fixed. I assume somebody will pick up one of them...

Will
diff mbox series

Patch

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e5da394d1ca3..c9ec050bcf46 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -128,6 +128,7 @@  static int zero;
 static int __maybe_unused one = 1;
 static int __maybe_unused two = 2;
 static int __maybe_unused four = 4;
+static unsigned long zero_ul;
 static unsigned long one_ul = 1;
 static unsigned long long_max = LONG_MAX;
 static int one_hundred = 100;
@@ -1750,7 +1751,7 @@  static struct ctl_table fs_table[] = {
 		.maxlen		= sizeof(files_stat.max_files),
 		.mode		= 0644,
 		.proc_handler	= proc_doulongvec_minmax,
-		.extra1		= &zero,
+		.extra1		= &zero_ul,
 		.extra2		= &long_max,
 	},
 	{