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

[Christian Brauner <christian@brauner.io>]

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
>