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 <[email protected]>
Date: Fri Jul 25 01:48:08 2008 -0700
Later zero and one were used in a bunch of other ctl_table's.
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 <[email protected]>
> 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.
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 <[email protected]>
Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
Signed-off-by: Andrey Ryabinin <[email protected]>
---
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
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 <[email protected]>
> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
> Signed-off-by: Andrey Ryabinin <[email protected]>
> ---
> 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 <[email protected]>
On Wed, Dec 3, 2014 at 3:39 PM, Andrey Ryabinin <[email protected]> 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 <[email protected]>
>> 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;
On 12/03/2014 04:27 PM, Dmitry Vyukov wrote:
> On Wed, Dec 3, 2014 at 3:39 PM, Andrey Ryabinin <[email protected]> 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 <[email protected]>
>>> 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;
>
On Wed, Dec 3, 2014 at 4:37 PM, Andrey Ryabinin <[email protected]> wrote:
> On 12/03/2014 04:27 PM, Dmitry Vyukov wrote:
>> On Wed, Dec 3, 2014 at 3:39 PM, Andrey Ryabinin <[email protected]> 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 <[email protected]>
>>>> 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;
>>
>
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 <[email protected]>
> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
> Signed-off-by: Andrey Ryabinin <[email protected]>
> ---
> 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 <[email protected]>
On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <[email protected]> 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 <[email protected]>
> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
> Signed-off-by: Andrey Ryabinin <[email protected]>
> ---
> 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.
On Wed, 3 Dec 2014 15:25:24 -0800 Andrew Morton <[email protected]> wrote:
> On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <[email protected]> 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 <[email protected]>
> > Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
> > Signed-off-by: Andrey Ryabinin <[email protected]>
> > ---
> > 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.
Hi Andrew,
On 12/04/2014 12:25 AM, Andrew Morton wrote:
> On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <[email protected]> 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 <[email protected]>
>> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
>> Signed-off-by: Andrey Ryabinin <[email protected]>
>> ---
>> 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
On 12/04/2014 03:19 AM, Andrew Morton wrote:
> On Wed, 3 Dec 2014 15:25:24 -0800 Andrew Morton <[email protected]> wrote:
>
>> On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <[email protected]> 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 <[email protected]>
>>> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
>>> Signed-off-by: Andrey Ryabinin <[email protected]>
>>> ---
>>> 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.
On Thu, 04 Dec 2014 07:12:45 +0100 Manfred Spraul <[email protected]> wrote:
> Hi Andrew,
>
> On 12/04/2014 12:25 AM, Andrew Morton wrote:
> > On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <[email protected]> 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 <[email protected]>
> >> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
> >> Signed-off-by: Andrey Ryabinin <[email protected]>
> >> ---
> >> 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 - ...
Hi,
On 12/04/2014 12:25 AM, Andrew Morton wrote:
> On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <[email protected]> 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 <[email protected]>
>> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
>> Signed-off-by: Andrey Ryabinin <[email protected]>
>> ---
>> 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
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 <[email protected]> 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 <[email protected]>
>>> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
>>> Signed-off-by: Andrey Ryabinin <[email protected]>
>>> ---
>>> 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
>
>
>
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 <[email protected]>
Suggested-by: Manfred Spraul <[email protected]>
Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
Signed-off-by: Andrey Ryabinin <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> Suggested-by: Manfred Spraul <[email protected]>
> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity")
> Signed-off-by: Andrey Ryabinin <[email protected]>
Acked-by: David Rientjes <[email protected]>
Patch title is a little awkward, though, maybe "mm, hugetlb: remove
unnecessary lower bound on sysctl handlers"?
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 <[email protected]>
Acked-by: David Rientjes <[email protected]>