* Out-of-bounds access in __do_proc_doulongvec_minmax
@ 2014-12-03 9:04 Dmitry Vyukov
2014-12-03 12:39 ` Andrey Ryabinin
0 siblings, 1 reply; 23+ messages in thread
From: Dmitry Vyukov @ 2014-12-03 9:04 UTC (permalink / raw)
To: Andrew Morton, Nadia.Derbey, aquini, davidlohr, Joe Perches,
manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov,
Andrey Konovalov, Andrey Ryabinin, Konstantin Khlebnikov,
kasan-dev
Hi,
I am working on AddressSanitizer, a fast memory error detector for kernel:
https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel
Here is a bug report that I've got while running trinity:
==================================================================
BUG: AddressSanitizer: out of bounds access in
__do_proc_doulongvec_minmax+0x8a0/0x9a0 at addr ffffffff83980960
Read of size 8 by task trinity-c14/6919
Out-of-bounds access to the global variable 'zero'
[ffffffff83980960-ffffffff83980964) defined at ipc/ipc_sysctl.c:158
CPU: 1 PID: 6919 Comm: trinity-c14 Not tainted 3.18.0-rc1+ #50
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
0000000000000001 ffff8800b68cf418 ffffffff82c2d3ae 0000000000000000
ffff8800b68cf4c0 ffff8800b68cf4a8 ffffffff813eaa81 ffffffff0000000c
ffff88010b003600 ffff8800b68cf479 0000000000000296 0000000000000000
Call Trace:
[<ffffffff813ead71>] __asan_report_load8_noabort+0x51/0x70
mm/kasan/report.c:248
[<ffffffff810cc3e0>] __do_proc_doulongvec_minmax+0x8a0/0x9a0
kernel/sysctl.c:2284
[< inlined >] proc_doulongvec_minmax+0x50/0x80
do_proc_doulongvec_minmax kernel/sysctl.c:2322
[<ffffffff810cc530>] proc_doulongvec_minmax+0x50/0x80 kernel/sysctl.c:2345
[<ffffffff813c9e5a>] hugetlb_sysctl_handler_common+0x12a/0x3c0
mm/hugetlb.c:2270
[<ffffffff813cb45c>] hugetlb_mempolicy_sysctl_handler+0x1c/0x20
mm/hugetlb.c:2293
[<ffffffff8153e6e9>] proc_sys_call_handler+0x179/0x1f0
fs/proc/proc_sysctl.c:506
[<ffffffff8153e76f>] proc_sys_write+0xf/0x20 fs/proc/proc_sysctl.c:524
[<ffffffff813f1563>] __kernel_write+0x123/0x440 fs/read_write.c:502
[<ffffffff8147ebaa>] write_pipe_buf+0x14a/0x1d0 fs/splice.c:1074
[< inlined >] __splice_from_pipe+0x22e/0x6f0
splice_from_pipe_feed fs/splice.c:769
[<ffffffff8147dbde>] __splice_from_pipe+0x22e/0x6f0 fs/splice.c:886
[<ffffffff81483211>] splice_from_pipe+0xc1/0x110 fs/splice.c:921
[<ffffffff81483298>] default_file_splice_write+0x18/0x50 fs/splice.c:1086
[< inlined >] direct_splice_actor+0x104/0x1c0 do_splice_from
fs/splice.c:1128
[<ffffffff8147cfc4>] direct_splice_actor+0x104/0x1c0 fs/splice.c:1284
[<ffffffff8147e5ba>] splice_direct_to_actor+0x24a/0x6f0 fs/splice.c:1237
[<ffffffff81483424>] do_splice_direct+0x154/0x270 fs/splice.c:1327
[<ffffffff813f3bfb>] do_sendfile+0x5fb/0x1260 fs/read_write.c:1266
[< inlined >] SyS_sendfile64+0xfa/0x100 SYSC_sendfile64
fs/read_write.c:1327
[<ffffffff813f6bea>] SyS_sendfile64+0xfa/0x100 fs/read_write.c:1313
[<ffffffff82c464f9>] ia32_do_call+0x13/0x13 arch/x86/ia32/ia32entry.S:444
Memory state around the buggy address:
ffffffff83980680: 04 f8 f8 f8 f8 f8 f8 f8 02 f8 f8 f8 f8 f8 f8 f8
ffffffff83980700: 00 f8 f8 f8 f8 f8 f8 f8 00 f8 f8 f8 f8 f8 f8 f8
ffffffff83980780: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 00 00 00 00
ffffffff83980800: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 04 f8 f8 f8
ffffffff83980880: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8
>ffffffff83980900: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8
^
ffffffff83980980: f8 f8 f8 f8 00 00 00 00 f8 f8 f8 f8 00 00 00 00
ffffffff83980a00: 02 f8 f8 f8 f8 f8 f8 f8 00 00 00 00 00 00 00 00
ffffffff83980a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffffff83980b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffffff83980b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================
The core creates ctl_table as:
static int zero;
static int one = 1;
static int int_max = INT_MAX;
static struct ctl_table ipc_kern_table[] = {
{
...
{
.procname = "shm_rmid_forced",
.data = &init_ipc_ns.shm_rmid_forced,
.maxlen = sizeof(init_ipc_ns.shm_rmid_forced),
.mode = 0644,
.proc_handler = proc_ipc_dointvec_minmax_orphans,
.extra1 = &zero,
.extra2 = &one,
},
But later extra1/2 are casted to *unsigned long**:
static int __do_proc_doulongvec_minmax(void *data, struct ctl_table
*table, int write, ...
{
...
min = (unsigned long *) table->extra1;
max = (unsigned long *) table->extra2;
This leads to bogus bounds check for the sysctl value.
The bug is added in commit:
commit 9eefe520c814f6f62c5d36a2ddcd3fb99dfdb30e
Author: Nadia Derbey <Nadia.Derbey@bull.net>
Date: Fri Jul 25 01:48:08 2008 -0700
Later zero and one were used in a bunch of other ctl_table's.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Out-of-bounds access in __do_proc_doulongvec_minmax 2014-12-03 9:04 Out-of-bounds access in __do_proc_doulongvec_minmax Dmitry Vyukov @ 2014-12-03 12:39 ` Andrey Ryabinin 2014-12-03 12:41 ` [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable Andrey Ryabinin 2014-12-03 13:27 ` Out-of-bounds access in __do_proc_doulongvec_minmax Dmitry Vyukov 0 siblings, 2 replies; 23+ messages in thread From: Andrey Ryabinin @ 2014-12-03 12:39 UTC (permalink / raw) To: Dmitry Vyukov Cc: Andrew Morton, Nadia.Derbey, aquini, davidlohr, Joe Perches, manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov On 12/03/2014 12:04 PM, Dmitry Vyukov wrote: > Hi, > > I am working on AddressSanitizer, a fast memory error detector for kernel: > https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel > > Here is a bug report that I've got while running trinity: > > ================================================================== > BUG: AddressSanitizer: out of bounds access in > __do_proc_doulongvec_minmax+0x8a0/0x9a0 at addr ffffffff83980960 > Read of size 8 by task trinity-c14/6919 > Out-of-bounds access to the global variable 'zero' > [ffffffff83980960-ffffffff83980964) defined at ipc/ipc_sysctl.c:158 This line seems incorrect. Judging from the backtrace below variable 'zero' is defined in kernel/sysctl.c:123 > > CPU: 1 PID: 6919 Comm: trinity-c14 Not tainted 3.18.0-rc1+ #50 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > 0000000000000001 ffff8800b68cf418 ffffffff82c2d3ae 0000000000000000 > ffff8800b68cf4c0 ffff8800b68cf4a8 ffffffff813eaa81 ffffffff0000000c > ffff88010b003600 ffff8800b68cf479 0000000000000296 0000000000000000 > Call Trace: > [<ffffffff813ead71>] __asan_report_load8_noabort+0x51/0x70 > mm/kasan/report.c:248 > [<ffffffff810cc3e0>] __do_proc_doulongvec_minmax+0x8a0/0x9a0 > kernel/sysctl.c:2284 > [< inlined >] proc_doulongvec_minmax+0x50/0x80 > do_proc_doulongvec_minmax kernel/sysctl.c:2322 > [<ffffffff810cc530>] proc_doulongvec_minmax+0x50/0x80 kernel/sysctl.c:2345 > [<ffffffff813c9e5a>] hugetlb_sysctl_handler_common+0x12a/0x3c0 > mm/hugetlb.c:2270 > [<ffffffff813cb45c>] hugetlb_mempolicy_sysctl_handler+0x1c/0x20 > mm/hugetlb.c:2293 > [<ffffffff8153e6e9>] proc_sys_call_handler+0x179/0x1f0 > fs/proc/proc_sysctl.c:506 > [<ffffffff8153e76f>] proc_sys_write+0xf/0x20 fs/proc/proc_sysctl.c:524 > [<ffffffff813f1563>] __kernel_write+0x123/0x440 fs/read_write.c:502 > [<ffffffff8147ebaa>] write_pipe_buf+0x14a/0x1d0 fs/splice.c:1074 > [< inlined >] __splice_from_pipe+0x22e/0x6f0 > splice_from_pipe_feed fs/splice.c:769 > [<ffffffff8147dbde>] __splice_from_pipe+0x22e/0x6f0 fs/splice.c:886 > [<ffffffff81483211>] splice_from_pipe+0xc1/0x110 fs/splice.c:921 > [<ffffffff81483298>] default_file_splice_write+0x18/0x50 fs/splice.c:1086 > [< inlined >] direct_splice_actor+0x104/0x1c0 do_splice_from > fs/splice.c:1128 > [<ffffffff8147cfc4>] direct_splice_actor+0x104/0x1c0 fs/splice.c:1284 > [<ffffffff8147e5ba>] splice_direct_to_actor+0x24a/0x6f0 fs/splice.c:1237 > [<ffffffff81483424>] do_splice_direct+0x154/0x270 fs/splice.c:1327 > [<ffffffff813f3bfb>] do_sendfile+0x5fb/0x1260 fs/read_write.c:1266 > [< inlined >] SyS_sendfile64+0xfa/0x100 SYSC_sendfile64 > fs/read_write.c:1327 > [<ffffffff813f6bea>] SyS_sendfile64+0xfa/0x100 fs/read_write.c:1313 > [<ffffffff82c464f9>] ia32_do_call+0x13/0x13 arch/x86/ia32/ia32entry.S:444 > Memory state around the buggy address: > ffffffff83980680: 04 f8 f8 f8 f8 f8 f8 f8 02 f8 f8 f8 f8 f8 f8 f8 > ffffffff83980700: 00 f8 f8 f8 f8 f8 f8 f8 00 f8 f8 f8 f8 f8 f8 f8 > ffffffff83980780: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 00 00 00 00 > ffffffff83980800: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 04 f8 f8 f8 > ffffffff83980880: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 >> ffffffff83980900: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 > ^ > ffffffff83980980: f8 f8 f8 f8 00 00 00 00 f8 f8 f8 f8 00 00 00 00 > ffffffff83980a00: 02 f8 f8 f8 f8 f8 f8 f8 00 00 00 00 00 00 00 00 > ffffffff83980a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ffffffff83980b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ffffffff83980b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ================================================================== > > The core creates ctl_table as: > > static int zero; > static int one = 1; > static int int_max = INT_MAX; > static struct ctl_table ipc_kern_table[] = { > { > ... > { > .procname = "shm_rmid_forced", > .data = &init_ipc_ns.shm_rmid_forced, > .maxlen = sizeof(init_ipc_ns.shm_rmid_forced), > .mode = 0644, > .proc_handler = proc_ipc_dointvec_minmax_orphans, > .extra1 = &zero, > .extra2 = &one, > }, > > But later extra1/2 are casted to *unsigned long**: > > static int __do_proc_doulongvec_minmax(void *data, struct ctl_table > *table, int write, ... > { > ... > min = (unsigned long *) table->extra1; > max = (unsigned long *) table->extra2; > > This leads to bogus bounds check for the sysctl value. > > The bug is added in commit: > > commit 9eefe520c814f6f62c5d36a2ddcd3fb99dfdb30e > Author: Nadia Derbey <Nadia.Derbey@bull.net> > Date: Fri Jul 25 01:48:08 2008 -0700 > > Later zero and one were used in a bunch of other ctl_table's. > I think you are blaming wrong commit. This bug was introduced by ed4d4902ebdd7ca8b5a51daaf6bebf4b172895cc ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") We have two options to fix this. Reintroduce back hugetlb_zero or make 'zero' unsigned long instead. I would prefer the latter, changing type to 'unsigned long' shouldn't harm any other users of this variable. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable 2014-12-03 12:39 ` Andrey Ryabinin @ 2014-12-03 12:41 ` Andrey Ryabinin 2014-12-03 13:04 ` Rafael Aquini ` (2 more replies) 2014-12-03 13:27 ` Out-of-bounds access in __do_proc_doulongvec_minmax Dmitry Vyukov 1 sibling, 3 replies; 23+ messages in thread From: Andrey Ryabinin @ 2014-12-03 12:41 UTC (permalink / raw) To: akpm Cc: Andrey Ryabinin, Dmitry Vyukov, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, Nadia.Derbey, aquini, davidlohr, Joe Perches, manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev Commit ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") replaced 'unsigned long hugetlb_zero' with 'int zero' leading to out-of-bounds access in proc_doulongvec_minmax(): ================================================================== BUG: AddressSanitizer: out of bounds access in __do_proc_doulongvec_minmax+0x8a0/0x9a0 at addr ffffffff83980960 Read of size 8 by task trinity-c14/6919 Out-of-bounds access to the global variable 'zero' [ffffffff83980960-ffffffff83980964) defined at ipc/ipc_sysctl.c:158 CPU: 1 PID: 6919 Comm: trinity-c14 Not tainted 3.18.0-rc1+ #50 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 0000000000000001 ffff8800b68cf418 ffffffff82c2d3ae 0000000000000000 ffff8800b68cf4c0 ffff8800b68cf4a8 ffffffff813eaa81 ffffffff0000000c ffff88010b003600 ffff8800b68cf479 0000000000000296 0000000000000000 Call Trace: [<ffffffff813ead71>] __asan_report_load8_noabort+0x51/0x70 mm/kasan/report.c:248 [<ffffffff810cc3e0>] __do_proc_doulongvec_minmax+0x8a0/0x9a0 kernel/sysctl.c:2284 [< inlined >] proc_doulongvec_minmax+0x50/0x80 do_proc_doulongvec_minmax kernel/sysctl.c:2322 [<ffffffff810cc530>] proc_doulongvec_minmax+0x50/0x80 kernel/sysctl.c:2345 [<ffffffff813c9e5a>] hugetlb_sysctl_handler_common+0x12a/0x3c0 mm/hugetlb.c:2270 [<ffffffff813cb45c>] hugetlb_mempolicy_sysctl_handler+0x1c/0x20 mm/hugetlb.c:2293 [<ffffffff8153e6e9>] proc_sys_call_handler+0x179/0x1f0 fs/proc/proc_sysctl.c:506 [<ffffffff8153e76f>] proc_sys_write+0xf/0x20 fs/proc/proc_sysctl.c:524 [<ffffffff813f1563>] __kernel_write+0x123/0x440 fs/read_write.c:502 [<ffffffff8147ebaa>] write_pipe_buf+0x14a/0x1d0 fs/splice.c:1074 [< inlined >] __splice_from_pipe+0x22e/0x6f0 splice_from_pipe_feed fs/splice.c:769 [<ffffffff8147dbde>] __splice_from_pipe+0x22e/0x6f0 fs/splice.c:886 [<ffffffff81483211>] splice_from_pipe+0xc1/0x110 fs/splice.c:921 [<ffffffff81483298>] default_file_splice_write+0x18/0x50 fs/splice.c:1086 [< inlined >] direct_splice_actor+0x104/0x1c0 do_splice_from fs/splice.c:1128 [<ffffffff8147cfc4>] direct_splice_actor+0x104/0x1c0 fs/splice.c:1284 [<ffffffff8147e5ba>] splice_direct_to_actor+0x24a/0x6f0 fs/splice.c:1237 [<ffffffff81483424>] do_splice_direct+0x154/0x270 fs/splice.c:1327 [<ffffffff813f3bfb>] do_sendfile+0x5fb/0x1260 fs/read_write.c:1266 [< inlined >] SyS_sendfile64+0xfa/0x100 SYSC_sendfile64 fs/read_write.c:1327 [<ffffffff813f6bea>] SyS_sendfile64+0xfa/0x100 fs/read_write.c:1313 [<ffffffff82c464f9>] ia32_do_call+0x13/0x13 arch/x86/ia32/ia32entry.S:444 Memory state around the buggy address: ffffffff83980680: 04 f8 f8 f8 f8 f8 f8 f8 02 f8 f8 f8 f8 f8 f8 f8 ffffffff83980700: 00 f8 f8 f8 f8 f8 f8 f8 00 f8 f8 f8 f8 f8 f8 f8 ffffffff83980780: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 00 00 00 00 ffffffff83980800: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 04 f8 f8 f8 ffffffff83980880: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 >ffffffff83980900: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 ^ ffffffff83980980: f8 f8 f8 f8 00 00 00 00 f8 f8 f8 f8 00 00 00 00 ffffffff83980a00: 02 f8 f8 f8 f8 f8 f8 f8 00 00 00 00 00 00 00 00 ffffffff83980a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffffffff83980b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffffffff83980b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ================================================================== Use the 'unsigned long' type for 'zero' variable to fix this. Changing type to 'unsigned long' shouldn't affect any other users of this variable. Reported-by: Dmitry Vyukov <dvyukov@google.com> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> --- kernel/sysctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 15f2511..45c45c9 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -120,7 +120,7 @@ static int sixty = 60; static int __maybe_unused neg_one = -1; -static int zero; +static unsigned long zero; static int __maybe_unused one = 1; static int __maybe_unused two = 2; static int __maybe_unused four = 4; -- 2.2.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable 2014-12-03 12:41 ` [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable Andrey Ryabinin @ 2014-12-03 13:04 ` Rafael Aquini 2014-12-03 21:12 ` David Rientjes 2014-12-03 23:25 ` Andrew Morton 2 siblings, 0 replies; 23+ messages in thread From: Rafael Aquini @ 2014-12-03 13:04 UTC (permalink / raw) To: Andrey Ryabinin Cc: akpm, Dmitry Vyukov, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, Nadia.Derbey, davidlohr, Joe Perches, manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev On Wed, Dec 03, 2014 at 03:41:21PM +0300, Andrey Ryabinin wrote: > > Commit ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") replaced > 'unsigned long hugetlb_zero' with 'int zero' leading to out-of-bounds access > in proc_doulongvec_minmax(): > > ================================================================== > BUG: AddressSanitizer: out of bounds access in > __do_proc_doulongvec_minmax+0x8a0/0x9a0 at addr ffffffff83980960 > Read of size 8 by task trinity-c14/6919 > Out-of-bounds access to the global variable 'zero' > [ffffffff83980960-ffffffff83980964) defined at ipc/ipc_sysctl.c:158 > > CPU: 1 PID: 6919 Comm: trinity-c14 Not tainted 3.18.0-rc1+ #50 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > 0000000000000001 ffff8800b68cf418 ffffffff82c2d3ae 0000000000000000 > ffff8800b68cf4c0 ffff8800b68cf4a8 ffffffff813eaa81 ffffffff0000000c > ffff88010b003600 ffff8800b68cf479 0000000000000296 0000000000000000 > Call Trace: > [<ffffffff813ead71>] __asan_report_load8_noabort+0x51/0x70 > mm/kasan/report.c:248 > [<ffffffff810cc3e0>] __do_proc_doulongvec_minmax+0x8a0/0x9a0 > kernel/sysctl.c:2284 > [< inlined >] proc_doulongvec_minmax+0x50/0x80 > do_proc_doulongvec_minmax kernel/sysctl.c:2322 > [<ffffffff810cc530>] proc_doulongvec_minmax+0x50/0x80 kernel/sysctl.c:2345 > [<ffffffff813c9e5a>] hugetlb_sysctl_handler_common+0x12a/0x3c0 > mm/hugetlb.c:2270 > [<ffffffff813cb45c>] hugetlb_mempolicy_sysctl_handler+0x1c/0x20 > mm/hugetlb.c:2293 > [<ffffffff8153e6e9>] proc_sys_call_handler+0x179/0x1f0 > fs/proc/proc_sysctl.c:506 > [<ffffffff8153e76f>] proc_sys_write+0xf/0x20 fs/proc/proc_sysctl.c:524 > [<ffffffff813f1563>] __kernel_write+0x123/0x440 fs/read_write.c:502 > [<ffffffff8147ebaa>] write_pipe_buf+0x14a/0x1d0 fs/splice.c:1074 > [< inlined >] __splice_from_pipe+0x22e/0x6f0 > splice_from_pipe_feed fs/splice.c:769 > [<ffffffff8147dbde>] __splice_from_pipe+0x22e/0x6f0 fs/splice.c:886 > [<ffffffff81483211>] splice_from_pipe+0xc1/0x110 fs/splice.c:921 > [<ffffffff81483298>] default_file_splice_write+0x18/0x50 fs/splice.c:1086 > [< inlined >] direct_splice_actor+0x104/0x1c0 do_splice_from > fs/splice.c:1128 > [<ffffffff8147cfc4>] direct_splice_actor+0x104/0x1c0 fs/splice.c:1284 > [<ffffffff8147e5ba>] splice_direct_to_actor+0x24a/0x6f0 fs/splice.c:1237 > [<ffffffff81483424>] do_splice_direct+0x154/0x270 fs/splice.c:1327 > [<ffffffff813f3bfb>] do_sendfile+0x5fb/0x1260 fs/read_write.c:1266 > [< inlined >] SyS_sendfile64+0xfa/0x100 SYSC_sendfile64 > fs/read_write.c:1327 > [<ffffffff813f6bea>] SyS_sendfile64+0xfa/0x100 fs/read_write.c:1313 > [<ffffffff82c464f9>] ia32_do_call+0x13/0x13 arch/x86/ia32/ia32entry.S:444 > Memory state around the buggy address: > ffffffff83980680: 04 f8 f8 f8 f8 f8 f8 f8 02 f8 f8 f8 f8 f8 f8 f8 > ffffffff83980700: 00 f8 f8 f8 f8 f8 f8 f8 00 f8 f8 f8 f8 f8 f8 f8 > ffffffff83980780: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 00 00 00 00 > ffffffff83980800: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 04 f8 f8 f8 > ffffffff83980880: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 > >ffffffff83980900: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 > ^ > ffffffff83980980: f8 f8 f8 f8 00 00 00 00 f8 f8 f8 f8 00 00 00 00 > ffffffff83980a00: 02 f8 f8 f8 f8 f8 f8 f8 00 00 00 00 00 00 00 00 > ffffffff83980a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ffffffff83980b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ffffffff83980b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ================================================================== > > Use the 'unsigned long' type for 'zero' variable to fix this. > Changing type to 'unsigned long' shouldn't affect any other users > of this variable. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") > Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> > --- > kernel/sysctl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 15f2511..45c45c9 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -120,7 +120,7 @@ static int sixty = 60; > > static int __maybe_unused neg_one = -1; > > -static int zero; > +static unsigned long zero; > static int __maybe_unused one = 1; > static int __maybe_unused two = 2; > static int __maybe_unused four = 4; > -- > 2.2.0 > Acked-by: Rafael Aquini <aquini@redhat.com> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable 2014-12-03 12:41 ` [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable Andrey Ryabinin 2014-12-03 13:04 ` Rafael Aquini @ 2014-12-03 21:12 ` David Rientjes 2014-12-03 23:25 ` Andrew Morton 2 siblings, 0 replies; 23+ messages in thread From: David Rientjes @ 2014-12-03 21:12 UTC (permalink / raw) To: Andrey Ryabinin Cc: akpm, Dmitry Vyukov, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, Nadia.Derbey, aquini, davidlohr, Joe Perches, manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev On Wed, 3 Dec 2014, Andrey Ryabinin wrote: > > Commit ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") replaced > 'unsigned long hugetlb_zero' with 'int zero' leading to out-of-bounds access > in proc_doulongvec_minmax(): > > ================================================================== > BUG: AddressSanitizer: out of bounds access in > __do_proc_doulongvec_minmax+0x8a0/0x9a0 at addr ffffffff83980960 > Read of size 8 by task trinity-c14/6919 > Out-of-bounds access to the global variable 'zero' > [ffffffff83980960-ffffffff83980964) defined at ipc/ipc_sysctl.c:158 > > CPU: 1 PID: 6919 Comm: trinity-c14 Not tainted 3.18.0-rc1+ #50 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > 0000000000000001 ffff8800b68cf418 ffffffff82c2d3ae 0000000000000000 > ffff8800b68cf4c0 ffff8800b68cf4a8 ffffffff813eaa81 ffffffff0000000c > ffff88010b003600 ffff8800b68cf479 0000000000000296 0000000000000000 > Call Trace: > [<ffffffff813ead71>] __asan_report_load8_noabort+0x51/0x70 > mm/kasan/report.c:248 > [<ffffffff810cc3e0>] __do_proc_doulongvec_minmax+0x8a0/0x9a0 > kernel/sysctl.c:2284 > [< inlined >] proc_doulongvec_minmax+0x50/0x80 > do_proc_doulongvec_minmax kernel/sysctl.c:2322 > [<ffffffff810cc530>] proc_doulongvec_minmax+0x50/0x80 kernel/sysctl.c:2345 > [<ffffffff813c9e5a>] hugetlb_sysctl_handler_common+0x12a/0x3c0 > mm/hugetlb.c:2270 > [<ffffffff813cb45c>] hugetlb_mempolicy_sysctl_handler+0x1c/0x20 > mm/hugetlb.c:2293 > [<ffffffff8153e6e9>] proc_sys_call_handler+0x179/0x1f0 > fs/proc/proc_sysctl.c:506 > [<ffffffff8153e76f>] proc_sys_write+0xf/0x20 fs/proc/proc_sysctl.c:524 > [<ffffffff813f1563>] __kernel_write+0x123/0x440 fs/read_write.c:502 > [<ffffffff8147ebaa>] write_pipe_buf+0x14a/0x1d0 fs/splice.c:1074 > [< inlined >] __splice_from_pipe+0x22e/0x6f0 > splice_from_pipe_feed fs/splice.c:769 > [<ffffffff8147dbde>] __splice_from_pipe+0x22e/0x6f0 fs/splice.c:886 > [<ffffffff81483211>] splice_from_pipe+0xc1/0x110 fs/splice.c:921 > [<ffffffff81483298>] default_file_splice_write+0x18/0x50 fs/splice.c:1086 > [< inlined >] direct_splice_actor+0x104/0x1c0 do_splice_from > fs/splice.c:1128 > [<ffffffff8147cfc4>] direct_splice_actor+0x104/0x1c0 fs/splice.c:1284 > [<ffffffff8147e5ba>] splice_direct_to_actor+0x24a/0x6f0 fs/splice.c:1237 > [<ffffffff81483424>] do_splice_direct+0x154/0x270 fs/splice.c:1327 > [<ffffffff813f3bfb>] do_sendfile+0x5fb/0x1260 fs/read_write.c:1266 > [< inlined >] SyS_sendfile64+0xfa/0x100 SYSC_sendfile64 > fs/read_write.c:1327 > [<ffffffff813f6bea>] SyS_sendfile64+0xfa/0x100 fs/read_write.c:1313 > [<ffffffff82c464f9>] ia32_do_call+0x13/0x13 arch/x86/ia32/ia32entry.S:444 > Memory state around the buggy address: > ffffffff83980680: 04 f8 f8 f8 f8 f8 f8 f8 02 f8 f8 f8 f8 f8 f8 f8 > ffffffff83980700: 00 f8 f8 f8 f8 f8 f8 f8 00 f8 f8 f8 f8 f8 f8 f8 > ffffffff83980780: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 00 00 00 00 > ffffffff83980800: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 04 f8 f8 f8 > ffffffff83980880: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 > >ffffffff83980900: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 > ^ > ffffffff83980980: f8 f8 f8 f8 00 00 00 00 f8 f8 f8 f8 00 00 00 00 > ffffffff83980a00: 02 f8 f8 f8 f8 f8 f8 f8 00 00 00 00 00 00 00 00 > ffffffff83980a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ffffffff83980b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ffffffff83980b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ================================================================== > > Use the 'unsigned long' type for 'zero' variable to fix this. > Changing type to 'unsigned long' shouldn't affect any other users > of this variable. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") > Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> > --- > kernel/sysctl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 15f2511..45c45c9 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -120,7 +120,7 @@ static int sixty = 60; > > static int __maybe_unused neg_one = -1; > > -static int zero; > +static unsigned long zero; > static int __maybe_unused one = 1; > static int __maybe_unused two = 2; > static int __maybe_unused four = 4; __do_proc_doulongvec_minmax() casts both extra1 and extra2 to unsigned long *, so this is appropriate. Acked-by: David Rientjes <rientjes@google.com> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable 2014-12-03 12:41 ` [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable Andrey Ryabinin 2014-12-03 13:04 ` Rafael Aquini 2014-12-03 21:12 ` David Rientjes @ 2014-12-03 23:25 ` Andrew Morton 2014-12-04 0:19 ` Andrew Morton ` (2 more replies) 2 siblings, 3 replies; 23+ messages in thread From: Andrew Morton @ 2014-12-03 23:25 UTC (permalink / raw) To: Andrey Ryabinin Cc: Dmitry Vyukov, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, Nadia.Derbey, aquini, davidlohr, Joe Perches, manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <a.ryabinin@samsung.com> wrote: > > Use the 'unsigned long' type for 'zero' variable to fix this. > Changing type to 'unsigned long' shouldn't affect any other users > of this variable. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") > Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> > --- > kernel/sysctl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 15f2511..45c45c9 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -120,7 +120,7 @@ static int sixty = 60; > > static int __maybe_unused neg_one = -1; > > -static int zero; > +static unsigned long zero; > static int __maybe_unused one = 1; > static int __maybe_unused two = 2; > static int __maybe_unused four = 4; Yeah, this is ghastly. Look at { .procname = "numa_balancing", .data = NULL, /* filled in by handler */ .maxlen = sizeof(unsigned int), .mode = 0644, .proc_handler = sysctl_numa_balancing, .extra1 = &zero, .extra2 = &one, }, Now extra1 points at a long and extra2 points at an int. sysctl_numa_balancing() calls proc_dointvec_minmax() and I think your patch just broke big-endian 64-bit machines. "sched_autogroup_enabled" breaks as well. These sysctl tables drove a big truck straight through the C type system and enabled all sorts of nasty breakage. So how do we fix it? Maybe we could actually use the type system a bit and do something like union sysctl_payload { int i; unsigned long ul; ... }; static union zero_int = { .i = 0 }; static union zero_ulong = { .ul = 0 }; and change proc_dointvec_minmax() and a million other functions to take `union sysctl_payload *' arguments. But I haven't thought about it much. Even for a minimal fix, someone should go through each and every ctl_table and audit/fix the .extra1/.extra2 types/sizes. And until that's done I'm not inclined to apply anything, because at least the current code appears to work a bit. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable 2014-12-03 23:25 ` Andrew Morton @ 2014-12-04 0:19 ` Andrew Morton 2014-12-04 11:35 ` Andrey Ryabinin 2014-12-04 6:12 ` Manfred Spraul 2014-12-13 20:51 ` Manfred Spraul 2 siblings, 1 reply; 23+ messages in thread From: Andrew Morton @ 2014-12-04 0:19 UTC (permalink / raw) To: Andrey Ryabinin, Dmitry Vyukov, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, Nadia.Derbey, aquini, davidlohr, Joe Perches, manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev On Wed, 3 Dec 2014 15:25:24 -0800 Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <a.ryabinin@samsung.com> wrote: > > > > > Use the 'unsigned long' type for 'zero' variable to fix this. > > Changing type to 'unsigned long' shouldn't affect any other users > > of this variable. > > > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > > Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") > > Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> > > --- > > kernel/sysctl.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > > index 15f2511..45c45c9 100644 > > --- a/kernel/sysctl.c > > +++ b/kernel/sysctl.c > > @@ -120,7 +120,7 @@ static int sixty = 60; > > > > static int __maybe_unused neg_one = -1; > > > > -static int zero; > > +static unsigned long zero; > > static int __maybe_unused one = 1; > > static int __maybe_unused two = 2; > > static int __maybe_unused four = 4; > > Yeah, this is ghastly. > > Look at > > { > .procname = "numa_balancing", > .data = NULL, /* filled in by handler */ > .maxlen = sizeof(unsigned int), > .mode = 0644, > .proc_handler = sysctl_numa_balancing, > .extra1 = &zero, > .extra2 = &one, > }, > > Now extra1 points at a long and extra2 points at an int. > sysctl_numa_balancing() calls proc_dointvec_minmax() and I think your > patch just broke big-endian 64-bit machines. "sched_autogroup_enabled" > breaks as well. Taking another look at this... numa_balancing will continue to work on big-endian because of course zero is still zero when byteswapped. But that's such a hack, isn't documented and doesn't work for "one", "sixty", etc. I'm thinking a better fix here is to switch hugetlb_sysctl_handler to use `int's. 2^32 hugepages is enough for anybody. hugetlb_overcommit_handler() will need conversion also. Perhaps auditing all the proc_doulongvec_minmax callsites is the way to attack this. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable 2014-12-04 0:19 ` Andrew Morton @ 2014-12-04 11:35 ` Andrey Ryabinin 0 siblings, 0 replies; 23+ messages in thread From: Andrey Ryabinin @ 2014-12-04 11:35 UTC (permalink / raw) To: Andrew Morton, Dmitry Vyukov, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, Nadia.Derbey, aquini, Joe Perches, manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev On 12/04/2014 03:19 AM, Andrew Morton wrote: > On Wed, 3 Dec 2014 15:25:24 -0800 Andrew Morton <akpm@linux-foundation.org> wrote: > >> On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <a.ryabinin@samsung.com> wrote: >> >>> >>> Use the 'unsigned long' type for 'zero' variable to fix this. >>> Changing type to 'unsigned long' shouldn't affect any other users >>> of this variable. >>> >>> Reported-by: Dmitry Vyukov <dvyukov@google.com> >>> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") >>> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> >>> --- >>> kernel/sysctl.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >>> index 15f2511..45c45c9 100644 >>> --- a/kernel/sysctl.c >>> +++ b/kernel/sysctl.c >>> @@ -120,7 +120,7 @@ static int sixty = 60; >>> >>> static int __maybe_unused neg_one = -1; >>> >>> -static int zero; >>> +static unsigned long zero; >>> static int __maybe_unused one = 1; >>> static int __maybe_unused two = 2; >>> static int __maybe_unused four = 4; >> >> Yeah, this is ghastly. >> >> Look at >> >> { >> .procname = "numa_balancing", >> .data = NULL, /* filled in by handler */ >> .maxlen = sizeof(unsigned int), >> .mode = 0644, >> .proc_handler = sysctl_numa_balancing, >> .extra1 = &zero, >> .extra2 = &one, >> }, >> >> Now extra1 points at a long and extra2 points at an int. >> sysctl_numa_balancing() calls proc_dointvec_minmax() and I think your >> patch just broke big-endian 64-bit machines. "sched_autogroup_enabled" >> breaks as well. > > Taking another look at this... > > numa_balancing will continue to work on big-endian because of course > zero is still zero when byteswapped. But that's such a hack, isn't > documented and doesn't work for "one", "sixty", etc. > Yeah, I agree it's a bit hacky. > I'm thinking a better fix here is to switch hugetlb_sysctl_handler to > use `int's. 2^32 hugepages is enough for anybody. > It's 8 petabytes for 2MB pages, so yeah should be enough. Perhaps it also makes sense to change types for counters in 'struct hstate' from longs to ints. > hugetlb_overcommit_handler() will need conversion also. > > Perhaps auditing all the proc_doulongvec_minmax callsites is the way to > attack this. > I've looked through this yesterday and didn't found anything obviously wrong. Though I could easily miss something. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable 2014-12-03 23:25 ` Andrew Morton 2014-12-04 0:19 ` Andrew Morton @ 2014-12-04 6:12 ` Manfred Spraul 2014-12-05 22:50 ` Andrew Morton 2014-12-13 20:51 ` Manfred Spraul 2 siblings, 1 reply; 23+ messages in thread From: Manfred Spraul @ 2014-12-04 6:12 UTC (permalink / raw) To: Andrew Morton, Andrey Ryabinin Cc: Dmitry Vyukov, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, Nadia.Derbey, aquini, davidlohr, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev [-- Attachment #1: Type: text/plain, Size: 1892 bytes --] Hi Andrew, On 12/04/2014 12:25 AM, Andrew Morton wrote: > On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <a.ryabinin@samsung.com> wrote: > >> Use the 'unsigned long' type for 'zero' variable to fix this. >> Changing type to 'unsigned long' shouldn't affect any other users >> of this variable. >> >> Reported-by: Dmitry Vyukov <dvyukov@google.com> >> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") >> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> >> --- >> kernel/sysctl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index 15f2511..45c45c9 100644 >> --- a/kernel/sysctl.c >> +++ b/kernel/sysctl.c >> @@ -120,7 +120,7 @@ static int sixty = 60; >> >> static int __maybe_unused neg_one = -1; >> >> -static int zero; >> +static unsigned long zero; >> static int __maybe_unused one = 1; >> static int __maybe_unused two = 2; >> static int __maybe_unused four = 4; > Yeah, this is ghastly. > > Look at > > { > .procname = "numa_balancing", > .data = NULL, /* filled in by handler */ > .maxlen = sizeof(unsigned int), > .mode = 0644, > .proc_handler = sysctl_numa_balancing, > .extra1 = &zero, > .extra2 = &one, > }, > > Now extra1 points at a long and extra2 points at an int. > sysctl_numa_balancing() calls proc_dointvec_minmax() and I think your > patch just broke big-endian 64-bit machines. "sched_autogroup_enabled" > breaks as well. What about getting rid of "extra1" and "extra2" as well and replace it with "min" and "max"? I've attached an idea > and change proc_dointvec_minmax() and a million other functions to take > `union sysctl_payload *' arguments. But I haven't thought about it much. Another idea: why do we pass "int *" instead of "int"? With "int", we could use .int_min = 0; .int_max = 1; -- Manfred [-- Attachment #2: 0001-PATCH-Allow-type-safe-documented-sysctl.patch --] [-- Type: text/x-patch, Size: 4368 bytes --] >From 7a210bec3d9dc3382ef0d6809a7742856373bbee Mon Sep 17 00:00:00 2001 From: Manfred Spraul <manfred@colorfullife.com> Date: Thu, 4 Dec 2014 07:03:39 +0100 Subject: [PATCH] Allow type safe & documented sysctl Idea from Andrew: - add a union into struct ctl_table instead of the void * - further idea: replace "extra1" and "extra2" with min/max - use it for ipc Notes: - not tested - not coding style reviewed - open FIXME in ipc_sysctl.c Signed-off-by: Manfred Spraul <manfred@colorfullife.com> --- include/linux/sysctl.h | 16 ++++++++++++++-- ipc/ipc_sysctl.c | 34 ++++++++++++++++++---------------- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index b7361f8..acc7581 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -111,8 +111,20 @@ struct ctl_table struct ctl_table *child; /* Deprecated */ proc_handler *proc_handler; /* Callback for text formatting */ struct ctl_table_poll *poll; - void *extra1; - void *extra2; + union { + void *extra1; + int *int_min; + long *long_min; + unsigned int *uint_min; + unsigned long *ulong_min; + }; + union { + void *extra2; + int *int_max; + long *long_max; + unsigned int *uint_max; + unsigned long *ulong_max; + }; }; struct ctl_node { diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c index c3f0326..50a6e1c 100644 --- a/ipc/ipc_sysctl.c +++ b/ipc/ipc_sysctl.c @@ -167,6 +167,7 @@ static struct ctl_table ipc_kern_table[] = { .maxlen = sizeof(init_ipc_ns.shm_ctlmax), .mode = 0644, .proc_handler = proc_ipc_doulongvec_minmax, + /* FIXME: Why no ulong_min & ulong_max ?? */ }, { .procname = "shmall", @@ -174,6 +175,7 @@ static struct ctl_table ipc_kern_table[] = { .maxlen = sizeof(init_ipc_ns.shm_ctlall), .mode = 0644, .proc_handler = proc_ipc_doulongvec_minmax, + /* FIXME: Why no ulong_min & ulong_max ?? */ }, { .procname = "shmmni", @@ -188,8 +190,8 @@ static struct ctl_table ipc_kern_table[] = { .maxlen = sizeof(init_ipc_ns.shm_rmid_forced), .mode = 0644, .proc_handler = proc_ipc_dointvec_minmax_orphans, - .extra1 = &zero, - .extra2 = &one, + .int_min = &zero, + .int_max = &one, }, { .procname = "msgmax", @@ -197,8 +199,8 @@ static struct ctl_table ipc_kern_table[] = { .maxlen = sizeof(init_ipc_ns.msg_ctlmax), .mode = 0644, .proc_handler = proc_ipc_dointvec_minmax, - .extra1 = &zero, - .extra2 = &int_max, + .int_min = &zero, + .int_max = &int_max, }, { .procname = "msgmni", @@ -206,8 +208,8 @@ static struct ctl_table ipc_kern_table[] = { .maxlen = sizeof(init_ipc_ns.msg_ctlmni), .mode = 0644, .proc_handler = proc_ipc_callback_dointvec_minmax, - .extra1 = &zero, - .extra2 = &int_max, + .int_min = &zero, + .int_max = &int_max, }, { .procname = "msgmnb", @@ -215,8 +217,8 @@ static struct ctl_table ipc_kern_table[] = { .maxlen = sizeof(init_ipc_ns.msg_ctlmnb), .mode = 0644, .proc_handler = proc_ipc_dointvec_minmax, - .extra1 = &zero, - .extra2 = &int_max, + .int_min = &zero, + .int_max = &int_max, }, { .procname = "sem", @@ -231,8 +233,8 @@ static struct ctl_table ipc_kern_table[] = { .maxlen = sizeof(int), .mode = 0644, .proc_handler = proc_ipcauto_dointvec_minmax, - .extra1 = &zero, - .extra2 = &one, + .int_min = &zero, + .int_max = &one, }, #ifdef CONFIG_CHECKPOINT_RESTORE { @@ -241,8 +243,8 @@ static struct ctl_table ipc_kern_table[] = { .maxlen = sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id), .mode = 0644, .proc_handler = proc_ipc_dointvec_minmax, - .extra1 = &zero, - .extra2 = &int_max, + .int_min = &zero, + .int_max = &int_max, }, { .procname = "msg_next_id", @@ -250,8 +252,8 @@ static struct ctl_table ipc_kern_table[] = { .maxlen = sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id), .mode = 0644, .proc_handler = proc_ipc_dointvec_minmax, - .extra1 = &zero, - .extra2 = &int_max, + .int_min = &zero, + .int_max = &int_max, }, { .procname = "shm_next_id", @@ -259,8 +261,8 @@ static struct ctl_table ipc_kern_table[] = { .maxlen = sizeof(init_ipc_ns.ids[IPC_SHM_IDS].next_id), .mode = 0644, .proc_handler = proc_ipc_dointvec_minmax, - .extra1 = &zero, - .extra2 = &int_max, + .int_min = &zero, + .int_max = &int_max, }, #endif {} -- 1.9.3 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable 2014-12-04 6:12 ` Manfred Spraul @ 2014-12-05 22:50 ` Andrew Morton 0 siblings, 0 replies; 23+ messages in thread From: Andrew Morton @ 2014-12-05 22:50 UTC (permalink / raw) To: Manfred Spraul Cc: Andrey Ryabinin, Dmitry Vyukov, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, Nadia.Derbey, aquini, davidlohr, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev On Thu, 04 Dec 2014 07:12:45 +0100 Manfred Spraul <manfred@colorfullife.com> wrote: > Hi Andrew, > > On 12/04/2014 12:25 AM, Andrew Morton wrote: > > On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <a.ryabinin@samsung.com> wrote: > > > >> Use the 'unsigned long' type for 'zero' variable to fix this. > >> Changing type to 'unsigned long' shouldn't affect any other users > >> of this variable. > >> > >> Reported-by: Dmitry Vyukov <dvyukov@google.com> > >> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") > >> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> > >> --- > >> kernel/sysctl.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c > >> index 15f2511..45c45c9 100644 > >> --- a/kernel/sysctl.c > >> +++ b/kernel/sysctl.c > >> @@ -120,7 +120,7 @@ static int sixty = 60; > >> > >> static int __maybe_unused neg_one = -1; > >> > >> -static int zero; > >> +static unsigned long zero; > >> static int __maybe_unused one = 1; > >> static int __maybe_unused two = 2; > >> static int __maybe_unused four = 4; > > Yeah, this is ghastly. > > > > Look at > > > > { > > .procname = "numa_balancing", > > .data = NULL, /* filled in by handler */ > > .maxlen = sizeof(unsigned int), > > .mode = 0644, > > .proc_handler = sysctl_numa_balancing, > > .extra1 = &zero, > > .extra2 = &one, > > }, > > > > Now extra1 points at a long and extra2 points at an int. > > sysctl_numa_balancing() calls proc_dointvec_minmax() and I think your > > patch just broke big-endian 64-bit machines. "sched_autogroup_enabled" > > breaks as well. > What about getting rid of "extra1" and "extra2" as well and replace it > with "min" and "max"? > > I've attached an idea Looks sane. > > and change proc_dointvec_minmax() and a million other functions to take > > `union sysctl_payload *' arguments. But I haven't thought about it much. > Another idea: why do we pass "int *" instead of "int"? > > With "int", we could use > .int_min = 0; > .int_max = 1; Presumably they were originally made void* so they could point at any thing at all. But I don't recall seeing extra1 and extra2 used for anything other than bounds checking on a scalar. Problem is, these things aren't always compile-time constants. For example, pid_max_min and pid_max_max are altered at runtime. I doubt if we need to support both ints and longs in extra1/2 - longs should be OK for range-checking int values. The signed/unsigned issue needs thinking about - there's a "neg_one" in there. If we make everything "long" then we might run into signedness/range issues for sysctls which can have large unsigned values with the top bit set: 0x8000000-0xffffffff and 0x8000000000000000 - ... ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable 2014-12-03 23:25 ` Andrew Morton 2014-12-04 0:19 ` Andrew Morton 2014-12-04 6:12 ` Manfred Spraul @ 2014-12-13 20:51 ` Manfred Spraul 2014-12-15 6:41 ` Andrey Ryabinin 2014-12-17 14:30 ` Andrey Ryabinin 2 siblings, 2 replies; 23+ messages in thread From: Manfred Spraul @ 2014-12-13 20:51 UTC (permalink / raw) To: Andrey Ryabinin, Dmitry Vyukov Cc: Andrew Morton, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, Nadia.Derbey, aquini, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, Davidlohr Bueso [-- Attachment #1: Type: text/plain, Size: 1303 bytes --] Hi, On 12/04/2014 12:25 AM, Andrew Morton wrote: > On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <a.ryabinin@samsung.com> wrote: > >> Use the 'unsigned long' type for 'zero' variable to fix this. >> Changing type to 'unsigned long' shouldn't affect any other users >> of this variable. >> >> Reported-by: Dmitry Vyukov <dvyukov@google.com> >> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") >> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> >> --- >> kernel/sysctl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index 15f2511..45c45c9 100644 >> --- a/kernel/sysctl.c >> +++ b/kernel/sysctl.c >> @@ -120,7 +120,7 @@ static int sixty = 60; >> >> static int __maybe_unused neg_one = -1; >> >> -static int zero; >> +static unsigned long zero; After some (useless) playing around (see the attached patch): Using > .extra1=zero, for proc_doulongvec_minmax doesn't make any sense: __do_proc_doulongvec_minmax() internally contains > if ((min && val < *min) || (max && val > *max)) > continue; What about just deleting the offending .extra1=zero line? .extra1=NULL has the same effect as .extra1=&zero. -- Manfred [-- Attachment #2: 0001-kernel-sysctl.c-Type-safe-macros.patch --] [-- Type: text/x-patch, Size: 5277 bytes --] >From 194e5d4758bb30531bad0907f06f3518002cd8b4 Mon Sep 17 00:00:00 2001 From: Manfred Spraul <manfred@colorfullife.com> Date: Sat, 13 Dec 2014 21:25:27 +0100 Subject: [PATCH] kernel/sysctl.c: Type safe macros struct ctl_table is used for creating entries in e.g. /proc/sys/kernel. The structure contains three void * entries and a function pointer, thus there is the risk of incorrectly mixing types. The patch create a macro that prevents accidential mixing, it enforces that the type expected by the function pointer and the three void * are all of the same type. Notes: - From my first impression, most proc entries mix types, and it works, because (int)1 and (unsigned int)1 are identical. Thus I'm not sure if this is the right approach. - the code doesn't compile, it contains an intentional incompatible type What do you think? -- Manfred --- kernel/sysctl.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 97 insertions(+), 16 deletions(-) diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 15f2511..bc446a6 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -276,6 +276,101 @@ static int min_extfrag_threshold; static int max_extfrag_threshold = 1000; #endif +/* + * Type safe macros for creating the sysctl table entries: + * + * TODO: + * - 1) It works. + * I.e. it complains when _dointvec is used to assign + * to an unsigned int. Unfortunately, this is very common: + * * _minmax is used, with min=0 and e.g. max=int_max. + * * the variable is actually used as a boolean, thus it doesn't matter + * if the user space interface returns -1 or 0xffffffff. + * * unsigned int zero is used as min + * * ... + * Thus most error messages would be false positives. + * + * - 2) The error message is difficult to understand + * error: initializer element is not constant + */ + +#define ASSIGN_TYPE_SAFE(param, type) \ + __builtin_choose_expr(__builtin_types_compatible_p(typeof(param), type), \ + param, \ + /* The void expression results in a compile-time error \ + when assigning the result to something. */ \ + ((void)0) \ + ) + +#define SYSCTL_BUILD_INT_ENTRY_MINMAX(name, entry, min_ptr, max_ptr) \ + { \ + .procname = (name), \ + .data = ASSIGN_TYPE_SAFE(&(entry), int *), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = proc_dointvec_minmax, \ + .extra1 = ASSIGN_TYPE_SAFE( (min_ptr), int *), \ + .extra2 = ASSIGN_TYPE_SAFE( (max_ptr), int *) \ + } + +#define SYSCTL_BUILD_ULONG_ENTRY_MINMAX(name, entry, min_ptr, max_ptr) \ + { \ + .procname = (name), \ + .data = ASSIGN_TYPE_SAFE(&(entry), unsigned long *), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = proc_doulongvec_minmax, \ + .extra1 = ASSIGN_TYPE_SAFE( (min_ptr), unsigned long *), \ + .extra2 = ASSIGN_TYPE_SAFE( (max_ptr), unsigned long *) \ + } + +#define SYSCTL_BUILD_ENTRY_MINMAX(name, entry, min_ptr, max_ptr) \ + { \ + .procname = (name), \ + .data = &(entry), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = __builtin_choose_expr(__builtin_types_compatible_p(typeof( &(entry)), int *), \ + proc_dointvec_minmax, \ + __builtin_choose_expr(__builtin_types_compatible_p(typeof( &(entry)), unsigned long *), \ + proc_doulongvec_minmax, \ + ((void)0) ) ), \ + .extra1 = ASSIGN_TYPE_SAFE( (min_ptr), typeof( &(entry) )), \ + .extra2 = ASSIGN_TYPE_SAFE( (max_ptr), typeof( &(entry) )) \ + } + +#define SYSCTL_BUILD_INT_ENTRY(name, entry) \ + { \ + .procname = (name), \ + .data = ASSIGN_TYPE_SAFE( &(entry), int *), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = proc_dointvec, \ + } + +#define SYSCTL_BUILD_ULONG_ENTRY(name, entry) \ + { \ + .procname = (name), \ + .data = ASSIGN_TYPE_SAFE( &(entry), unsigned long *), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = proc_doulongvec, \ + } + +#define SYSCTL_BUILD_ENTRY(name, entry) \ + { \ + .procname = (name), \ + .data = &(entry), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = __builtin_choose_expr(__builtin_types_compatible_p(typeof( &(entry)), int *), \ + proc_dointvec, \ + __builtin_choose_expr(__builtin_types_compatible_p(typeof( &(entry)), unsigned long *), \ + proc_doulongvec, \ + ((void)0) ) ) \ + } + + static struct ctl_table kern_table[] = { { .procname = "sched_child_runs_first", @@ -1344,22 +1439,8 @@ static struct ctl_table vm_table[] = { .mode = 0644, .proc_handler = proc_dointvec_jiffies, }, - { - .procname = "block_dump", - .data = &block_dump, - .maxlen = sizeof(block_dump), - .mode = 0644, - .proc_handler = proc_dointvec, - .extra1 = &zero, - }, - { - .procname = "vfs_cache_pressure", - .data = &sysctl_vfs_cache_pressure, - .maxlen = sizeof(sysctl_vfs_cache_pressure), - .mode = 0644, - .proc_handler = proc_dointvec, - .extra1 = &zero, - }, + SYSCTL_BUILD_ENTRY_MINMAX("block_dump", block_dump, &zero, (unsigned int *)NULL), + SYSCTL_BUILD_ENTRY_MINMAX("vfs_cache_pressure", sysctl_vfs_cache_pressure, &zero, (int *)NULL), #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT { .procname = "legacy_va_layout", -- 2.1.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable 2014-12-13 20:51 ` Manfred Spraul @ 2014-12-15 6:41 ` Andrey Ryabinin 2014-12-17 14:30 ` Andrey Ryabinin 1 sibling, 0 replies; 23+ messages in thread From: Andrey Ryabinin @ 2014-12-15 6:41 UTC (permalink / raw) To: Manfred Spraul, Dmitry Vyukov Cc: Andrew Morton, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, Nadia.Derbey, aquini, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, Davidlohr Bueso On 12/13/2014 11:51 PM, Manfred Spraul wrote: > Hi, > > On 12/04/2014 12:25 AM, Andrew Morton wrote: >> On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <a.ryabinin@samsung.com> wrote: >> >>> Use the 'unsigned long' type for 'zero' variable to fix this. >>> Changing type to 'unsigned long' shouldn't affect any other users >>> of this variable. >>> >>> Reported-by: Dmitry Vyukov <dvyukov@google.com> >>> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") >>> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> >>> --- >>> kernel/sysctl.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >>> index 15f2511..45c45c9 100644 >>> --- a/kernel/sysctl.c >>> +++ b/kernel/sysctl.c >>> @@ -120,7 +120,7 @@ static int sixty = 60; >>> static int __maybe_unused neg_one = -1; >>> -static int zero; >>> +static unsigned long zero; > > After some (useless) playing around (see the attached patch): > > Using >> .extra1=zero, > for proc_doulongvec_minmax doesn't make any sense: > > __do_proc_doulongvec_minmax() internally contains >> if ((min && val < *min) || (max && val > *max)) >> continue; > > What about just deleting the offending .extra1=zero line? > .extra1=NULL has the same effect as .extra1=&zero. > Agreed, I think this should work. > -- > Manfred > > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/2] hugetlb, sysctl: pass '.extra1 = NULL' rather then '.extra1 = &zero' 2014-12-13 20:51 ` Manfred Spraul @ 2014-12-17 14:30 ` Andrey Ryabinin 2014-12-17 14:30 ` Andrey Ryabinin 1 sibling, 0 replies; 23+ messages in thread From: Andrey Ryabinin @ 2014-12-17 14:30 UTC (permalink / raw) To: akpm Cc: Andrey Ryabinin, Dmitry Vyukov, Manfred Spraul, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, nadia.derbey@bull.net, aquini, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, Davidlohr Bueso, linux-mm Commit ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") replaced 'unsigned long hugetlb_zero' with 'int zero' leading to out-of-bounds access in proc_doulongvec_minmax(). Use '.extra1 = NULL' instead of '.extra1 = &zero'. Passing NULL is equivalent to passing minimal value, which is 0 for unsigned types. Reported-by: Dmitry Vyukov <dvyukov@google.com> Suggested-by: Manfred Spraul <manfred@colorfullife.com> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> --- kernel/sysctl.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 137c7f6..88ea2d6 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -1248,7 +1248,6 @@ static struct ctl_table vm_table[] = { .maxlen = sizeof(unsigned long), .mode = 0644, .proc_handler = hugetlb_sysctl_handler, - .extra1 = &zero, }, #ifdef CONFIG_NUMA { @@ -1257,7 +1256,6 @@ static struct ctl_table vm_table[] = { .maxlen = sizeof(unsigned long), .mode = 0644, .proc_handler = &hugetlb_mempolicy_sysctl_handler, - .extra1 = &zero, }, #endif { @@ -1280,7 +1278,6 @@ static struct ctl_table vm_table[] = { .maxlen = sizeof(unsigned long), .mode = 0644, .proc_handler = hugetlb_overcommit_handler, - .extra1 = &zero, }, #endif { -- 2.2.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 1/2] hugetlb, sysctl: pass '.extra1 = NULL' rather then '.extra1 = &zero' @ 2014-12-17 14:30 ` Andrey Ryabinin 0 siblings, 0 replies; 23+ messages in thread From: Andrey Ryabinin @ 2014-12-17 14:30 UTC (permalink / raw) To: akpm Cc: Andrey Ryabinin, Dmitry Vyukov, Manfred Spraul, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, nadia.derbey@bull.net, aquini, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, Davidlohr Bueso, linux-mm Commit ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") replaced 'unsigned long hugetlb_zero' with 'int zero' leading to out-of-bounds access in proc_doulongvec_minmax(). Use '.extra1 = NULL' instead of '.extra1 = &zero'. Passing NULL is equivalent to passing minimal value, which is 0 for unsigned types. Reported-by: Dmitry Vyukov <dvyukov@google.com> Suggested-by: Manfred Spraul <manfred@colorfullife.com> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> --- kernel/sysctl.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 137c7f6..88ea2d6 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -1248,7 +1248,6 @@ static struct ctl_table vm_table[] = { .maxlen = sizeof(unsigned long), .mode = 0644, .proc_handler = hugetlb_sysctl_handler, - .extra1 = &zero, }, #ifdef CONFIG_NUMA { @@ -1257,7 +1256,6 @@ static struct ctl_table vm_table[] = { .maxlen = sizeof(unsigned long), .mode = 0644, .proc_handler = &hugetlb_mempolicy_sysctl_handler, - .extra1 = &zero, }, #endif { @@ -1280,7 +1278,6 @@ static struct ctl_table vm_table[] = { .maxlen = sizeof(unsigned long), .mode = 0644, .proc_handler = hugetlb_overcommit_handler, - .extra1 = &zero, }, #endif { -- 2.2.0 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/2] mm: hugetlb: fix type of hugetlb_treat_as_movable variable 2014-12-17 14:30 ` Andrey Ryabinin @ 2014-12-17 14:30 ` Andrey Ryabinin -1 siblings, 0 replies; 23+ messages in thread From: Andrey Ryabinin @ 2014-12-17 14:30 UTC (permalink / raw) To: akpm Cc: Andrey Ryabinin, Dmitry Vyukov, Manfred Spraul, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, nadia.derbey@bull.net, aquini, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, Davidlohr Bueso, linux-mm hugetlb_treat_as_movable declared as unsigned long, but proc_dointvec() used for parsing it: static struct ctl_table vm_table[] = { ... { .procname = "hugepages_treat_as_movable", .data = &hugepages_treat_as_movable, .maxlen = sizeof(int), .mode = 0644, .proc_handler = proc_dointvec, }, This seems harmless, but it's better to use int type here. Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> --- include/linux/hugetlb.h | 2 +- mm/hugetlb.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 431b7fc..7d78563 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -86,7 +86,7 @@ void free_huge_page(struct page *page); pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud); #endif -extern unsigned long hugepages_treat_as_movable; +extern int hugepages_treat_as_movable; extern int sysctl_hugetlb_shm_group; extern struct list_head huge_boot_pages; diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 85032de..be0e5d0 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -35,7 +35,7 @@ #include <linux/node.h> #include "internal.h" -unsigned long hugepages_treat_as_movable; +int hugepages_treat_as_movable; int hugetlb_max_hstate __read_mostly; unsigned int default_hstate_idx; -- 2.2.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/2] mm: hugetlb: fix type of hugetlb_treat_as_movable variable @ 2014-12-17 14:30 ` Andrey Ryabinin 0 siblings, 0 replies; 23+ messages in thread From: Andrey Ryabinin @ 2014-12-17 14:30 UTC (permalink / raw) To: akpm Cc: Andrey Ryabinin, Dmitry Vyukov, Manfred Spraul, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, nadia.derbey@bull.net, aquini, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, Davidlohr Bueso, linux-mm hugetlb_treat_as_movable declared as unsigned long, but proc_dointvec() used for parsing it: static struct ctl_table vm_table[] = { ... { .procname = "hugepages_treat_as_movable", .data = &hugepages_treat_as_movable, .maxlen = sizeof(int), .mode = 0644, .proc_handler = proc_dointvec, }, This seems harmless, but it's better to use int type here. Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> --- include/linux/hugetlb.h | 2 +- mm/hugetlb.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 431b7fc..7d78563 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -86,7 +86,7 @@ void free_huge_page(struct page *page); pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud); #endif -extern unsigned long hugepages_treat_as_movable; +extern int hugepages_treat_as_movable; extern int sysctl_hugetlb_shm_group; extern struct list_head huge_boot_pages; diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 85032de..be0e5d0 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -35,7 +35,7 @@ #include <linux/node.h> #include "internal.h" -unsigned long hugepages_treat_as_movable; +int hugepages_treat_as_movable; int hugetlb_max_hstate __read_mostly; unsigned int default_hstate_idx; -- 2.2.0 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] mm: hugetlb: fix type of hugetlb_treat_as_movable variable 2014-12-17 14:30 ` Andrey Ryabinin @ 2014-12-18 0:39 ` David Rientjes -1 siblings, 0 replies; 23+ messages in thread From: David Rientjes @ 2014-12-18 0:39 UTC (permalink / raw) To: Andrey Ryabinin Cc: akpm, Dmitry Vyukov, Manfred Spraul, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, nadia.derbey@bull.net, aquini, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, Davidlohr Bueso, linux-mm On Wed, 17 Dec 2014, Andrey Ryabinin wrote: > hugetlb_treat_as_movable declared as unsigned long, but > proc_dointvec() used for parsing it: > > static struct ctl_table vm_table[] = { > ... > { > .procname = "hugepages_treat_as_movable", > .data = &hugepages_treat_as_movable, > .maxlen = sizeof(int), > .mode = 0644, > .proc_handler = proc_dointvec, > }, > > This seems harmless, but it's better to use int type here. > > Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> Acked-by: David Rientjes <rientjes@google.com> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] mm: hugetlb: fix type of hugetlb_treat_as_movable variable @ 2014-12-18 0:39 ` David Rientjes 0 siblings, 0 replies; 23+ messages in thread From: David Rientjes @ 2014-12-18 0:39 UTC (permalink / raw) To: Andrey Ryabinin Cc: akpm, Dmitry Vyukov, Manfred Spraul, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, nadia.derbey@bull.net, aquini, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, Davidlohr Bueso, linux-mm On Wed, 17 Dec 2014, Andrey Ryabinin wrote: > hugetlb_treat_as_movable declared as unsigned long, but > proc_dointvec() used for parsing it: > > static struct ctl_table vm_table[] = { > ... > { > .procname = "hugepages_treat_as_movable", > .data = &hugepages_treat_as_movable, > .maxlen = sizeof(int), > .mode = 0644, > .proc_handler = proc_dointvec, > }, > > This seems harmless, but it's better to use int type here. > > Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> Acked-by: David Rientjes <rientjes@google.com> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] hugetlb, sysctl: pass '.extra1 = NULL' rather then '.extra1 = &zero' 2014-12-17 14:30 ` Andrey Ryabinin @ 2014-12-18 0:38 ` David Rientjes -1 siblings, 0 replies; 23+ messages in thread From: David Rientjes @ 2014-12-18 0:38 UTC (permalink / raw) To: Andrey Ryabinin Cc: akpm, Dmitry Vyukov, Manfred Spraul, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, nadia.derbey@bull.net, aquini, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, Davidlohr Bueso, linux-mm On Wed, 17 Dec 2014, Andrey Ryabinin wrote: > Commit ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") replaced > 'unsigned long hugetlb_zero' with 'int zero' leading to out-of-bounds access > in proc_doulongvec_minmax(). > Use '.extra1 = NULL' instead of '.extra1 = &zero'. Passing NULL is equivalent to > passing minimal value, which is 0 for unsigned types. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Suggested-by: Manfred Spraul <manfred@colorfullife.com> > Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") > Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> Acked-by: David Rientjes <rientjes@google.com> Patch title is a little awkward, though, maybe "mm, hugetlb: remove unnecessary lower bound on sysctl handlers"? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] hugetlb, sysctl: pass '.extra1 = NULL' rather then '.extra1 = &zero' @ 2014-12-18 0:38 ` David Rientjes 0 siblings, 0 replies; 23+ messages in thread From: David Rientjes @ 2014-12-18 0:38 UTC (permalink / raw) To: Andrey Ryabinin Cc: akpm, Dmitry Vyukov, Manfred Spraul, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov, nadia.derbey@bull.net, aquini, Joe Perches, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, Davidlohr Bueso, linux-mm On Wed, 17 Dec 2014, Andrey Ryabinin wrote: > Commit ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") replaced > 'unsigned long hugetlb_zero' with 'int zero' leading to out-of-bounds access > in proc_doulongvec_minmax(). > Use '.extra1 = NULL' instead of '.extra1 = &zero'. Passing NULL is equivalent to > passing minimal value, which is 0 for unsigned types. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Suggested-by: Manfred Spraul <manfred@colorfullife.com> > Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") > Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> Acked-by: David Rientjes <rientjes@google.com> Patch title is a little awkward, though, maybe "mm, hugetlb: remove unnecessary lower bound on sysctl handlers"? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Out-of-bounds access in __do_proc_doulongvec_minmax 2014-12-03 12:39 ` Andrey Ryabinin 2014-12-03 12:41 ` [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable Andrey Ryabinin @ 2014-12-03 13:27 ` Dmitry Vyukov 2014-12-03 13:37 ` Andrey Ryabinin 1 sibling, 1 reply; 23+ messages in thread From: Dmitry Vyukov @ 2014-12-03 13:27 UTC (permalink / raw) To: Andrey Ryabinin Cc: Andrew Morton, nadia.derbey, aquini, davidlohr, Joe Perches, manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov On Wed, Dec 3, 2014 at 3:39 PM, Andrey Ryabinin <a.ryabinin@samsung.com> wrote: > On 12/03/2014 12:04 PM, Dmitry Vyukov wrote: >> Hi, >> >> I am working on AddressSanitizer, a fast memory error detector for kernel: >> https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel >> >> Here is a bug report that I've got while running trinity: >> >> ================================================================== >> BUG: AddressSanitizer: out of bounds access in >> __do_proc_doulongvec_minmax+0x8a0/0x9a0 at addr ffffffff83980960 >> Read of size 8 by task trinity-c14/6919 >> Out-of-bounds access to the global variable 'zero' >> [ffffffff83980960-ffffffff83980964) defined at ipc/ipc_sysctl.c:158 > > This line seems incorrect. Judging from the backtrace below variable 'zero' is > defined in kernel/sysctl.c:123 > > >> >> CPU: 1 PID: 6919 Comm: trinity-c14 Not tainted 3.18.0-rc1+ #50 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >> 0000000000000001 ffff8800b68cf418 ffffffff82c2d3ae 0000000000000000 >> ffff8800b68cf4c0 ffff8800b68cf4a8 ffffffff813eaa81 ffffffff0000000c >> ffff88010b003600 ffff8800b68cf479 0000000000000296 0000000000000000 >> Call Trace: >> [<ffffffff813ead71>] __asan_report_load8_noabort+0x51/0x70 >> mm/kasan/report.c:248 >> [<ffffffff810cc3e0>] __do_proc_doulongvec_minmax+0x8a0/0x9a0 >> kernel/sysctl.c:2284 >> [< inlined >] proc_doulongvec_minmax+0x50/0x80 >> do_proc_doulongvec_minmax kernel/sysctl.c:2322 >> [<ffffffff810cc530>] proc_doulongvec_minmax+0x50/0x80 kernel/sysctl.c:2345 >> [<ffffffff813c9e5a>] hugetlb_sysctl_handler_common+0x12a/0x3c0 >> mm/hugetlb.c:2270 >> [<ffffffff813cb45c>] hugetlb_mempolicy_sysctl_handler+0x1c/0x20 >> mm/hugetlb.c:2293 >> [<ffffffff8153e6e9>] proc_sys_call_handler+0x179/0x1f0 >> fs/proc/proc_sysctl.c:506 >> [<ffffffff8153e76f>] proc_sys_write+0xf/0x20 fs/proc/proc_sysctl.c:524 >> [<ffffffff813f1563>] __kernel_write+0x123/0x440 fs/read_write.c:502 >> [<ffffffff8147ebaa>] write_pipe_buf+0x14a/0x1d0 fs/splice.c:1074 >> [< inlined >] __splice_from_pipe+0x22e/0x6f0 >> splice_from_pipe_feed fs/splice.c:769 >> [<ffffffff8147dbde>] __splice_from_pipe+0x22e/0x6f0 fs/splice.c:886 >> [<ffffffff81483211>] splice_from_pipe+0xc1/0x110 fs/splice.c:921 >> [<ffffffff81483298>] default_file_splice_write+0x18/0x50 fs/splice.c:1086 >> [< inlined >] direct_splice_actor+0x104/0x1c0 do_splice_from >> fs/splice.c:1128 >> [<ffffffff8147cfc4>] direct_splice_actor+0x104/0x1c0 fs/splice.c:1284 >> [<ffffffff8147e5ba>] splice_direct_to_actor+0x24a/0x6f0 fs/splice.c:1237 >> [<ffffffff81483424>] do_splice_direct+0x154/0x270 fs/splice.c:1327 >> [<ffffffff813f3bfb>] do_sendfile+0x5fb/0x1260 fs/read_write.c:1266 >> [< inlined >] SyS_sendfile64+0xfa/0x100 SYSC_sendfile64 >> fs/read_write.c:1327 >> [<ffffffff813f6bea>] SyS_sendfile64+0xfa/0x100 fs/read_write.c:1313 >> [<ffffffff82c464f9>] ia32_do_call+0x13/0x13 arch/x86/ia32/ia32entry.S:444 >> Memory state around the buggy address: >> ffffffff83980680: 04 f8 f8 f8 f8 f8 f8 f8 02 f8 f8 f8 f8 f8 f8 f8 >> ffffffff83980700: 00 f8 f8 f8 f8 f8 f8 f8 00 f8 f8 f8 f8 f8 f8 f8 >> ffffffff83980780: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 00 00 00 00 >> ffffffff83980800: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 04 f8 f8 f8 >> ffffffff83980880: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 >>> ffffffff83980900: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 >> ^ >> ffffffff83980980: f8 f8 f8 f8 00 00 00 00 f8 f8 f8 f8 00 00 00 00 >> ffffffff83980a00: 02 f8 f8 f8 f8 f8 f8 f8 00 00 00 00 00 00 00 00 >> ffffffff83980a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> ffffffff83980b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> ffffffff83980b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> ================================================================== >> >> The core creates ctl_table as: >> >> static int zero; >> static int one = 1; >> static int int_max = INT_MAX; >> static struct ctl_table ipc_kern_table[] = { >> { >> ... >> { >> .procname = "shm_rmid_forced", >> .data = &init_ipc_ns.shm_rmid_forced, >> .maxlen = sizeof(init_ipc_ns.shm_rmid_forced), >> .mode = 0644, >> .proc_handler = proc_ipc_dointvec_minmax_orphans, >> .extra1 = &zero, >> .extra2 = &one, >> }, >> >> But later extra1/2 are casted to *unsigned long**: >> >> static int __do_proc_doulongvec_minmax(void *data, struct ctl_table >> *table, int write, ... >> { >> ... >> min = (unsigned long *) table->extra1; >> max = (unsigned long *) table->extra2; >> >> This leads to bogus bounds check for the sysctl value. >> >> The bug is added in commit: >> >> commit 9eefe520c814f6f62c5d36a2ddcd3fb99dfdb30e >> Author: Nadia Derbey <Nadia.Derbey@bull.net> >> Date: Fri Jul 25 01:48:08 2008 -0700 >> >> Later zero and one were used in a bunch of other ctl_table's. >> > > I think you are blaming wrong commit. This bug was introduced by > ed4d4902ebdd7ca8b5a51daaf6bebf4b172895cc ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") > > We have two options to fix this. Reintroduce back hugetlb_zero or make 'zero' unsigned long instead. > I would prefer the latter, changing type to 'unsigned long' shouldn't harm any other users of this variable. > ipc/ipc_sysctl.c also contains zero, one and int_max variables that are used in a similar way: static int zero; static int one = 1; static int int_max = INT_MAX; ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Out-of-bounds access in __do_proc_doulongvec_minmax 2014-12-03 13:27 ` Out-of-bounds access in __do_proc_doulongvec_minmax Dmitry Vyukov @ 2014-12-03 13:37 ` Andrey Ryabinin 2014-12-03 13:39 ` Dmitry Vyukov 0 siblings, 1 reply; 23+ messages in thread From: Andrey Ryabinin @ 2014-12-03 13:37 UTC (permalink / raw) To: Dmitry Vyukov Cc: Andrew Morton, nadia.derbey, aquini, davidlohr, Joe Perches, manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov On 12/03/2014 04:27 PM, Dmitry Vyukov wrote: > On Wed, Dec 3, 2014 at 3:39 PM, Andrey Ryabinin <a.ryabinin@samsung.com> wrote: >> On 12/03/2014 12:04 PM, Dmitry Vyukov wrote: >>> Hi, >>> >>> I am working on AddressSanitizer, a fast memory error detector for kernel: >>> https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel >>> >>> Here is a bug report that I've got while running trinity: >>> >>> ================================================================== >>> BUG: AddressSanitizer: out of bounds access in >>> __do_proc_doulongvec_minmax+0x8a0/0x9a0 at addr ffffffff83980960 >>> Read of size 8 by task trinity-c14/6919 >>> Out-of-bounds access to the global variable 'zero' >>> [ffffffff83980960-ffffffff83980964) defined at ipc/ipc_sysctl.c:158 >> >> This line seems incorrect. Judging from the backtrace below variable 'zero' is >> defined in kernel/sysctl.c:123 >> >> >>> >>> CPU: 1 PID: 6919 Comm: trinity-c14 Not tainted 3.18.0-rc1+ #50 >>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >>> 0000000000000001 ffff8800b68cf418 ffffffff82c2d3ae 0000000000000000 >>> ffff8800b68cf4c0 ffff8800b68cf4a8 ffffffff813eaa81 ffffffff0000000c >>> ffff88010b003600 ffff8800b68cf479 0000000000000296 0000000000000000 >>> Call Trace: >>> [<ffffffff813ead71>] __asan_report_load8_noabort+0x51/0x70 >>> mm/kasan/report.c:248 >>> [<ffffffff810cc3e0>] __do_proc_doulongvec_minmax+0x8a0/0x9a0 >>> kernel/sysctl.c:2284 >>> [< inlined >] proc_doulongvec_minmax+0x50/0x80 >>> do_proc_doulongvec_minmax kernel/sysctl.c:2322 >>> [<ffffffff810cc530>] proc_doulongvec_minmax+0x50/0x80 kernel/sysctl.c:2345 >>> [<ffffffff813c9e5a>] hugetlb_sysctl_handler_common+0x12a/0x3c0 >>> mm/hugetlb.c:2270 >>> [<ffffffff813cb45c>] hugetlb_mempolicy_sysctl_handler+0x1c/0x20 >>> mm/hugetlb.c:2293 >>> [<ffffffff8153e6e9>] proc_sys_call_handler+0x179/0x1f0 >>> fs/proc/proc_sysctl.c:506 >>> [<ffffffff8153e76f>] proc_sys_write+0xf/0x20 fs/proc/proc_sysctl.c:524 >>> [<ffffffff813f1563>] __kernel_write+0x123/0x440 fs/read_write.c:502 >>> [<ffffffff8147ebaa>] write_pipe_buf+0x14a/0x1d0 fs/splice.c:1074 >>> [< inlined >] __splice_from_pipe+0x22e/0x6f0 >>> splice_from_pipe_feed fs/splice.c:769 >>> [<ffffffff8147dbde>] __splice_from_pipe+0x22e/0x6f0 fs/splice.c:886 >>> [<ffffffff81483211>] splice_from_pipe+0xc1/0x110 fs/splice.c:921 >>> [<ffffffff81483298>] default_file_splice_write+0x18/0x50 fs/splice.c:1086 >>> [< inlined >] direct_splice_actor+0x104/0x1c0 do_splice_from >>> fs/splice.c:1128 >>> [<ffffffff8147cfc4>] direct_splice_actor+0x104/0x1c0 fs/splice.c:1284 >>> [<ffffffff8147e5ba>] splice_direct_to_actor+0x24a/0x6f0 fs/splice.c:1237 >>> [<ffffffff81483424>] do_splice_direct+0x154/0x270 fs/splice.c:1327 >>> [<ffffffff813f3bfb>] do_sendfile+0x5fb/0x1260 fs/read_write.c:1266 >>> [< inlined >] SyS_sendfile64+0xfa/0x100 SYSC_sendfile64 >>> fs/read_write.c:1327 >>> [<ffffffff813f6bea>] SyS_sendfile64+0xfa/0x100 fs/read_write.c:1313 >>> [<ffffffff82c464f9>] ia32_do_call+0x13/0x13 arch/x86/ia32/ia32entry.S:444 >>> Memory state around the buggy address: >>> ffffffff83980680: 04 f8 f8 f8 f8 f8 f8 f8 02 f8 f8 f8 f8 f8 f8 f8 >>> ffffffff83980700: 00 f8 f8 f8 f8 f8 f8 f8 00 f8 f8 f8 f8 f8 f8 f8 >>> ffffffff83980780: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 00 00 00 00 >>> ffffffff83980800: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 04 f8 f8 f8 >>> ffffffff83980880: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 >>>> ffffffff83980900: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 >>> ^ >>> ffffffff83980980: f8 f8 f8 f8 00 00 00 00 f8 f8 f8 f8 00 00 00 00 >>> ffffffff83980a00: 02 f8 f8 f8 f8 f8 f8 f8 00 00 00 00 00 00 00 00 >>> ffffffff83980a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> ffffffff83980b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> ffffffff83980b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> ================================================================== >>> >>> The core creates ctl_table as: >>> >>> static int zero; >>> static int one = 1; >>> static int int_max = INT_MAX; >>> static struct ctl_table ipc_kern_table[] = { >>> { >>> ... >>> { >>> .procname = "shm_rmid_forced", >>> .data = &init_ipc_ns.shm_rmid_forced, >>> .maxlen = sizeof(init_ipc_ns.shm_rmid_forced), >>> .mode = 0644, >>> .proc_handler = proc_ipc_dointvec_minmax_orphans, >>> .extra1 = &zero, >>> .extra2 = &one, >>> }, >>> >>> But later extra1/2 are casted to *unsigned long**: >>> >>> static int __do_proc_doulongvec_minmax(void *data, struct ctl_table >>> *table, int write, ... >>> { >>> ... >>> min = (unsigned long *) table->extra1; >>> max = (unsigned long *) table->extra2; >>> >>> This leads to bogus bounds check for the sysctl value. >>> >>> The bug is added in commit: >>> >>> commit 9eefe520c814f6f62c5d36a2ddcd3fb99dfdb30e >>> Author: Nadia Derbey <Nadia.Derbey@bull.net> >>> Date: Fri Jul 25 01:48:08 2008 -0700 >>> >>> Later zero and one were used in a bunch of other ctl_table's. >>> >> >> I think you are blaming wrong commit. This bug was introduced by >> ed4d4902ebdd7ca8b5a51daaf6bebf4b172895cc ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") >> >> We have two options to fix this. Reintroduce back hugetlb_zero or make 'zero' unsigned long instead. >> I would prefer the latter, changing type to 'unsigned long' shouldn't harm any other users of this variable. >> > > ipc/ipc_sysctl.c also contains zero, one and int_max variables that > are used in a similar way: > Yes, but they all look correct to me. Proc handlers in ipc/ipc_sysctl.c use proc_dointvec_minmax() so they should be fine with 'int zero'. But hugetlb_sysctl_handler_common() calls proc_doulongvec_minmax(), therefore it needs 'unsigned long'. > static int zero; > static int one = 1; > static int int_max = INT_MAX; > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Out-of-bounds access in __do_proc_doulongvec_minmax 2014-12-03 13:37 ` Andrey Ryabinin @ 2014-12-03 13:39 ` Dmitry Vyukov 0 siblings, 0 replies; 23+ messages in thread From: Dmitry Vyukov @ 2014-12-03 13:39 UTC (permalink / raw) To: Andrey Ryabinin Cc: Andrew Morton, nadia.derbey, aquini, davidlohr, Joe Perches, manfred, avagin, LKML, Kostya Serebryany, Dmitry Chernenkov, Andrey Konovalov, Konstantin Khlebnikov, kasan-dev, David Rientjes, Naoya Horiguchi, Luiz Capitulino, Kirill A. Shutemov On Wed, Dec 3, 2014 at 4:37 PM, Andrey Ryabinin <a.ryabinin@samsung.com> wrote: > On 12/03/2014 04:27 PM, Dmitry Vyukov wrote: >> On Wed, Dec 3, 2014 at 3:39 PM, Andrey Ryabinin <a.ryabinin@samsung.com> wrote: >>> On 12/03/2014 12:04 PM, Dmitry Vyukov wrote: >>>> Hi, >>>> >>>> I am working on AddressSanitizer, a fast memory error detector for kernel: >>>> https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel >>>> >>>> Here is a bug report that I've got while running trinity: >>>> >>>> ================================================================== >>>> BUG: AddressSanitizer: out of bounds access in >>>> __do_proc_doulongvec_minmax+0x8a0/0x9a0 at addr ffffffff83980960 >>>> Read of size 8 by task trinity-c14/6919 >>>> Out-of-bounds access to the global variable 'zero' >>>> [ffffffff83980960-ffffffff83980964) defined at ipc/ipc_sysctl.c:158 >>> >>> This line seems incorrect. Judging from the backtrace below variable 'zero' is >>> defined in kernel/sysctl.c:123 >>> >>> >>>> >>>> CPU: 1 PID: 6919 Comm: trinity-c14 Not tainted 3.18.0-rc1+ #50 >>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >>>> 0000000000000001 ffff8800b68cf418 ffffffff82c2d3ae 0000000000000000 >>>> ffff8800b68cf4c0 ffff8800b68cf4a8 ffffffff813eaa81 ffffffff0000000c >>>> ffff88010b003600 ffff8800b68cf479 0000000000000296 0000000000000000 >>>> Call Trace: >>>> [<ffffffff813ead71>] __asan_report_load8_noabort+0x51/0x70 >>>> mm/kasan/report.c:248 >>>> [<ffffffff810cc3e0>] __do_proc_doulongvec_minmax+0x8a0/0x9a0 >>>> kernel/sysctl.c:2284 >>>> [< inlined >] proc_doulongvec_minmax+0x50/0x80 >>>> do_proc_doulongvec_minmax kernel/sysctl.c:2322 >>>> [<ffffffff810cc530>] proc_doulongvec_minmax+0x50/0x80 kernel/sysctl.c:2345 >>>> [<ffffffff813c9e5a>] hugetlb_sysctl_handler_common+0x12a/0x3c0 >>>> mm/hugetlb.c:2270 >>>> [<ffffffff813cb45c>] hugetlb_mempolicy_sysctl_handler+0x1c/0x20 >>>> mm/hugetlb.c:2293 >>>> [<ffffffff8153e6e9>] proc_sys_call_handler+0x179/0x1f0 >>>> fs/proc/proc_sysctl.c:506 >>>> [<ffffffff8153e76f>] proc_sys_write+0xf/0x20 fs/proc/proc_sysctl.c:524 >>>> [<ffffffff813f1563>] __kernel_write+0x123/0x440 fs/read_write.c:502 >>>> [<ffffffff8147ebaa>] write_pipe_buf+0x14a/0x1d0 fs/splice.c:1074 >>>> [< inlined >] __splice_from_pipe+0x22e/0x6f0 >>>> splice_from_pipe_feed fs/splice.c:769 >>>> [<ffffffff8147dbde>] __splice_from_pipe+0x22e/0x6f0 fs/splice.c:886 >>>> [<ffffffff81483211>] splice_from_pipe+0xc1/0x110 fs/splice.c:921 >>>> [<ffffffff81483298>] default_file_splice_write+0x18/0x50 fs/splice.c:1086 >>>> [< inlined >] direct_splice_actor+0x104/0x1c0 do_splice_from >>>> fs/splice.c:1128 >>>> [<ffffffff8147cfc4>] direct_splice_actor+0x104/0x1c0 fs/splice.c:1284 >>>> [<ffffffff8147e5ba>] splice_direct_to_actor+0x24a/0x6f0 fs/splice.c:1237 >>>> [<ffffffff81483424>] do_splice_direct+0x154/0x270 fs/splice.c:1327 >>>> [<ffffffff813f3bfb>] do_sendfile+0x5fb/0x1260 fs/read_write.c:1266 >>>> [< inlined >] SyS_sendfile64+0xfa/0x100 SYSC_sendfile64 >>>> fs/read_write.c:1327 >>>> [<ffffffff813f6bea>] SyS_sendfile64+0xfa/0x100 fs/read_write.c:1313 >>>> [<ffffffff82c464f9>] ia32_do_call+0x13/0x13 arch/x86/ia32/ia32entry.S:444 >>>> Memory state around the buggy address: >>>> ffffffff83980680: 04 f8 f8 f8 f8 f8 f8 f8 02 f8 f8 f8 f8 f8 f8 f8 >>>> ffffffff83980700: 00 f8 f8 f8 f8 f8 f8 f8 00 f8 f8 f8 f8 f8 f8 f8 >>>> ffffffff83980780: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 00 00 00 00 >>>> ffffffff83980800: 00 00 00 00 00 00 00 00 f8 f8 f8 f8 04 f8 f8 f8 >>>> ffffffff83980880: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 >>>>> ffffffff83980900: f8 f8 f8 f8 04 f8 f8 f8 f8 f8 f8 f8 04 f8 f8 f8 >>>> ^ >>>> ffffffff83980980: f8 f8 f8 f8 00 00 00 00 f8 f8 f8 f8 00 00 00 00 >>>> ffffffff83980a00: 02 f8 f8 f8 f8 f8 f8 f8 00 00 00 00 00 00 00 00 >>>> ffffffff83980a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>>> ffffffff83980b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>>> ffffffff83980b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>>> ================================================================== >>>> >>>> The core creates ctl_table as: >>>> >>>> static int zero; >>>> static int one = 1; >>>> static int int_max = INT_MAX; >>>> static struct ctl_table ipc_kern_table[] = { >>>> { >>>> ... >>>> { >>>> .procname = "shm_rmid_forced", >>>> .data = &init_ipc_ns.shm_rmid_forced, >>>> .maxlen = sizeof(init_ipc_ns.shm_rmid_forced), >>>> .mode = 0644, >>>> .proc_handler = proc_ipc_dointvec_minmax_orphans, >>>> .extra1 = &zero, >>>> .extra2 = &one, >>>> }, >>>> >>>> But later extra1/2 are casted to *unsigned long**: >>>> >>>> static int __do_proc_doulongvec_minmax(void *data, struct ctl_table >>>> *table, int write, ... >>>> { >>>> ... >>>> min = (unsigned long *) table->extra1; >>>> max = (unsigned long *) table->extra2; >>>> >>>> This leads to bogus bounds check for the sysctl value. >>>> >>>> The bug is added in commit: >>>> >>>> commit 9eefe520c814f6f62c5d36a2ddcd3fb99dfdb30e >>>> Author: Nadia Derbey <Nadia.Derbey@bull.net> >>>> Date: Fri Jul 25 01:48:08 2008 -0700 >>>> >>>> Later zero and one were used in a bunch of other ctl_table's. >>>> >>> >>> I think you are blaming wrong commit. This bug was introduced by >>> ed4d4902ebdd7ca8b5a51daaf6bebf4b172895cc ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") >>> >>> We have two options to fix this. Reintroduce back hugetlb_zero or make 'zero' unsigned long instead. >>> I would prefer the latter, changing type to 'unsigned long' shouldn't harm any other users of this variable. >>> >> >> ipc/ipc_sysctl.c also contains zero, one and int_max variables that >> are used in a similar way: >> > > Yes, but they all look correct to me. Proc handlers in ipc/ipc_sysctl.c > use proc_dointvec_minmax() so they should be fine with 'int zero'. Ah, OK, I think you are right. I actually had a bug in the code that determines exact global involved. So it is very possible that it's another 'zero'. > But hugetlb_sysctl_handler_common() calls proc_doulongvec_minmax(), therefore it needs 'unsigned long'. > >> static int zero; >> static int one = 1; >> static int int_max = INT_MAX; >> > ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2014-12-18 0:40 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-12-03 9:04 Out-of-bounds access in __do_proc_doulongvec_minmax Dmitry Vyukov 2014-12-03 12:39 ` Andrey Ryabinin 2014-12-03 12:41 ` [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable Andrey Ryabinin 2014-12-03 13:04 ` Rafael Aquini 2014-12-03 21:12 ` David Rientjes 2014-12-03 23:25 ` Andrew Morton 2014-12-04 0:19 ` Andrew Morton 2014-12-04 11:35 ` Andrey Ryabinin 2014-12-04 6:12 ` Manfred Spraul 2014-12-05 22:50 ` Andrew Morton 2014-12-13 20:51 ` Manfred Spraul 2014-12-15 6:41 ` Andrey Ryabinin 2014-12-17 14:30 ` [PATCH 1/2] hugetlb, sysctl: pass '.extra1 = NULL' rather then '.extra1 = &zero' Andrey Ryabinin 2014-12-17 14:30 ` Andrey Ryabinin 2014-12-17 14:30 ` [PATCH 2/2] mm: hugetlb: fix type of hugetlb_treat_as_movable variable Andrey Ryabinin 2014-12-17 14:30 ` Andrey Ryabinin 2014-12-18 0:39 ` David Rientjes 2014-12-18 0:39 ` David Rientjes 2014-12-18 0:38 ` [PATCH 1/2] hugetlb, sysctl: pass '.extra1 = NULL' rather then '.extra1 = &zero' David Rientjes 2014-12-18 0:38 ` David Rientjes 2014-12-03 13:27 ` Out-of-bounds access in __do_proc_doulongvec_minmax Dmitry Vyukov 2014-12-03 13:37 ` Andrey Ryabinin 2014-12-03 13:39 ` Dmitry Vyukov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.