Out-of-bounds access in __do_proc_doulongvec_minmax

Out-of-bounds access in __do_proc_doulongvec_minmax

[Dmitry Vyukov]

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.

Re: Out-of-bounds access in __do_proc_doulongvec_minmax

[Andrey Ryabinin]

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.

[PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[Andrey Ryabinin]

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

Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[Rafael Aquini]

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>

Re: Out-of-bounds access in __do_proc_doulongvec_minmax

[Dmitry Vyukov]

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;

Re: Out-of-bounds access in __do_proc_doulongvec_minmax

[Andrey Ryabinin]

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;
> 

Re: Out-of-bounds access in __do_proc_doulongvec_minmax

[Dmitry Vyukov]

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;
>>
>

Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[David Rientjes]

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>

Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[Andrew Morton]

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.

Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[Andrew Morton]

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.

Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[Manfred Spraul]

[-- 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

Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[Andrey Ryabinin]

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.

Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[Andrew Morton]

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 - ...

Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[Manfred Spraul]

[-- 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

Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[Andrey Ryabinin]

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
> 
> 
> 

[PATCH 1/2] hugetlb, sysctl: pass '.extra1 = NULL' rather then '.extra1 = &zero'

[Andrey Ryabinin]

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

[PATCH 1/2] hugetlb, sysctl: pass '.extra1 = NULL' rather then '.extra1 = &zero'

[Andrey Ryabinin]

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>

[PATCH 2/2] mm: hugetlb: fix type of hugetlb_treat_as_movable variable

[Andrey Ryabinin]

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

[PATCH 2/2] mm: hugetlb: fix type of hugetlb_treat_as_movable variable

[Andrey Ryabinin]

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>

Re: [PATCH 1/2] hugetlb, sysctl: pass '.extra1 = NULL' rather then '.extra1 = &zero'

[David Rientjes]

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"?

Re: [PATCH 1/2] hugetlb, sysctl: pass '.extra1 = NULL' rather then '.extra1 = &zero'

[David Rientjes]

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>

Re: [PATCH 2/2] mm: hugetlb: fix type of hugetlb_treat_as_movable variable

[David Rientjes]

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>

Re: [PATCH 2/2] mm: hugetlb: fix type of hugetlb_treat_as_movable variable

[David Rientjes]

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>