Messages in this thread | | | Date | Wed, 3 Apr 2019 17:40:45 +0200 | From | Christian Brauner <> | Subject | Re: [PATCH] kernel/sysctl.c: Fix out-of-bounds access when setting file-max |
| |
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 >
| |