* [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
@ 2019-03-28 13:03 Matteo Croce
2019-04-03 11:54 ` Matteo Croce
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Matteo Croce @ 2019-03-28 13:03 UTC (permalink / raw)
To: linux-fsdevel; +Cc: LKML, Luis Chamberlain, Kees Cook
fs.file-max sysctl uses proc_doulongvec_minmax() as proc handler, which
accesses *extra1 and *extra2 as unsigned long, but commit 32a5ad9c2285
("sysctl: handle overflow for file-max") assigns &zero, which is an int,
to extra1, generating the following KASAN report.
Fix this by changing 'zero' to long, which does not need to be duplicated
like 'one' and 'one_ul' for two data types.
==================================================================
BUG: KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax+0x2f9/0x600
Read of size 8 at addr ffffffff8233dc20 by task systemd/1
CPU: 0 PID: 1 Comm: systemd Not tainted 5.1.0-rc2-kvm+ #22
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS ?-20180724_192412-buildhw-07.phx2.fedoraproject.org-1.fc29 04/01/2014
Call Trace:
print_address_description+0x67/0x23d
kasan_report.cold.3+0x1c/0x36
__do_proc_doulongvec_minmax+0x2f9/0x600
proc_doulongvec_minmax+0x3a/0x50
proc_sys_call_handler+0x11d/0x170
vfs_write+0xd7/0x200
ksys_write+0x93/0x110
do_syscall_64+0x57/0x140
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f67d33e8804
Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 5e 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53
RSP: 002b:00007fffd9992ed8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f67d33e8804
RDX: 0000000000000015 RSI: 00005586ce2607b0 RDI: 0000000000000004
RBP: 00007fffd9992f30 R08: 000000000000c0c0 R09: ffffffffffff0000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
R13: 0000000000000015 R14: 00005586ce2607c4 R15: 00007fffd9992f70
The buggy address belongs to the variable:
0xffffffff8233dc20
Memory state around the buggy address:
ffffffff8233db00: 00 00 00 00 00 00 00 00 fa fa fa fa 04 fa fa fa
ffffffff8233db80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa
>ffffffff8233dc00: fa fa fa fa 04 fa fa fa fa fa fa fa 00 00 00 00
^
ffffffff8233dc80: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
ffffffff8233dd00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================
Fixes: 32a5ad9c2285 ("sysctl: handle overflow for file-max")
Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
kernel/sysctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e5da394d1ca3..3e959d67d619 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -124,7 +124,7 @@ static int sixty = 60;
static int __maybe_unused neg_one = -1;
-static int zero;
+static long zero;
static int __maybe_unused one = 1;
static int __maybe_unused two = 2;
static int __maybe_unused four = 4;
--
2.20.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
2019-03-28 13:03 [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max Matteo Croce
@ 2019-04-03 11:54 ` Matteo Croce
2019-04-03 14:02 ` Christian Brauner
2019-04-03 17:41 ` Kees Cook
2 siblings, 0 replies; 11+ messages in thread
From: Matteo Croce @ 2019-04-03 11:54 UTC (permalink / raw)
To: linux-fsdevel
Cc: LKML, Luis Chamberlain, Kees Cook, Christian Brauner,
Matthew Wilcox, Zev Weiss
On Thu, Mar 28, 2019 at 2:03 PM Matteo Croce <mcroce@redhat.com> wrote:
>
> fs.file-max sysctl uses proc_doulongvec_minmax() as proc handler, which
> accesses *extra1 and *extra2 as unsigned long, but commit 32a5ad9c2285
> ("sysctl: handle overflow for file-max") assigns &zero, which is an int,
> to extra1, generating the following KASAN report.
> Fix this by changing 'zero' to long, which does not need to be duplicated
> like 'one' and 'one_ul' for two data types.
Hi,
Anyone looked at this patch? Does my fix looks sane?
Regards,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
2019-03-28 13:03 [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max Matteo Croce
2019-04-03 11:54 ` Matteo Croce
@ 2019-04-03 14:02 ` Christian Brauner
2019-04-03 15:24 ` Matteo Croce
2019-04-03 17:41 ` Kees Cook
2 siblings, 1 reply; 11+ messages in thread
From: Christian Brauner @ 2019-04-03 14:02 UTC (permalink / raw)
To: Matteo Croce
Cc: linux-fsdevel, LKML, Luis Chamberlain, Kees Cook, willy, zev, akpm
On Thu, Mar 28, 2019 at 02:03:06PM +0100, Matteo Croce wrote:
> fs.file-max sysctl uses proc_doulongvec_minmax() as proc handler, which
> accesses *extra1 and *extra2 as unsigned long, but commit 32a5ad9c2285
> ("sysctl: handle overflow for file-max") assigns &zero, which is an int,
> to extra1, generating the following KASAN report.
> Fix this by changing 'zero' to long, which does not need to be duplicated
> like 'one' and 'one_ul' for two data types.
Yeah, maybe but it still feels cleaner and more obvious to just add:
static long long_zero;
given that most callers actually seem to want an (unsigned) int.
I don't have a strong opinion though so if others feel that it's just a
waste of space consider it acked.
>
> ==================================================================
> BUG: KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax+0x2f9/0x600
> Read of size 8 at addr ffffffff8233dc20 by task systemd/1
>
> CPU: 0 PID: 1 Comm: systemd Not tainted 5.1.0-rc2-kvm+ #22
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS ?-20180724_192412-buildhw-07.phx2.fedoraproject.org-1.fc29 04/01/2014
> Call Trace:
> print_address_description+0x67/0x23d
> kasan_report.cold.3+0x1c/0x36
> __do_proc_doulongvec_minmax+0x2f9/0x600
> proc_doulongvec_minmax+0x3a/0x50
> proc_sys_call_handler+0x11d/0x170
> vfs_write+0xd7/0x200
> ksys_write+0x93/0x110
> do_syscall_64+0x57/0x140
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7f67d33e8804
> Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 5e 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53
> RSP: 002b:00007fffd9992ed8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f67d33e8804
> RDX: 0000000000000015 RSI: 00005586ce2607b0 RDI: 0000000000000004
> RBP: 00007fffd9992f30 R08: 000000000000c0c0 R09: ffffffffffff0000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
> R13: 0000000000000015 R14: 00005586ce2607c4 R15: 00007fffd9992f70
>
> The buggy address belongs to the variable:
> 0xffffffff8233dc20
>
> Memory state around the buggy address:
> ffffffff8233db00: 00 00 00 00 00 00 00 00 fa fa fa fa 04 fa fa fa
> ffffffff8233db80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa
> >ffffffff8233dc00: fa fa fa fa 04 fa fa fa fa fa fa fa 00 00 00 00
> ^
> ffffffff8233dc80: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
> ffffffff8233dd00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ==================================================================
>
> Fixes: 32a5ad9c2285 ("sysctl: handle overflow for file-max")
Next time, please take the time to Cc the author of the Fixes patch as
well whose commit this is fixing right away.
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---
> kernel/sysctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index e5da394d1ca3..3e959d67d619 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -124,7 +124,7 @@ static int sixty = 60;
>
> static int __maybe_unused neg_one = -1;
>
> -static int zero;
> +static long zero;
> static int __maybe_unused one = 1;
> static int __maybe_unused two = 2;
> static int __maybe_unused four = 4;
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
2019-04-03 14:02 ` Christian Brauner
@ 2019-04-03 15:24 ` Matteo Croce
2019-04-03 15:51 ` Matthew Wilcox
0 siblings, 1 reply; 11+ messages in thread
From: Matteo Croce @ 2019-04-03 15:24 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, LKML, Luis Chamberlain, Kees Cook, Matthew Wilcox,
Zev Weiss, Andrew Morton
On Wed, Apr 3, 2019 at 4:02 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Thu, Mar 28, 2019 at 02:03:06PM +0100, Matteo Croce wrote:
> > fs.file-max sysctl uses proc_doulongvec_minmax() as proc handler, which
> > accesses *extra1 and *extra2 as unsigned long, but commit 32a5ad9c2285
> > ("sysctl: handle overflow for file-max") assigns &zero, which is an int,
> > to extra1, generating the following KASAN report.
> > Fix this by changing 'zero' to long, which does not need to be duplicated
> > like 'one' and 'one_ul' for two data types.
>
> Yeah, maybe but it still feels cleaner and more obvious to just add:
>
> static long long_zero;
>
> given that most callers actually seem to want an (unsigned) int.
>
> I don't have a strong opinion though so if others feel that it's just a
> waste of space consider it acked.
>
Well, given that the value is zero, in this expectional case we could
avoid duplicating the symbol and save 4 bytes.
What the maintainers think?
Cheers,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
2019-04-03 15:24 ` Matteo Croce
@ 2019-04-03 15:51 ` Matthew Wilcox
2019-04-03 16:40 ` Matteo Croce
0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2019-04-03 15:51 UTC (permalink / raw)
To: Matteo Croce
Cc: Christian Brauner, linux-fsdevel, LKML, Luis Chamberlain,
Kees Cook, Zev Weiss, Andrew Morton
On Wed, Apr 03, 2019 at 05:24:26PM +0200, Matteo Croce wrote:
> On Wed, Apr 3, 2019 at 4:02 PM Christian Brauner <christian@brauner.io> wrote:
> > Yeah, maybe but it still feels cleaner and more obvious to just add:
> >
> > static long long_zero;
> >
> > given that most callers actually seem to want an (unsigned) int.
> >
> > I don't have a strong opinion though so if others feel that it's just a
> > waste of space consider it acked.
> >
>
> Well, given that the value is zero, in this expectional case we could
> avoid duplicating the symbol and save 4 bytes.
> What the maintainers think?
If we care about saving four bytes, we could just pass the address of
ZERO_PAGE(0).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
2019-04-03 15:51 ` Matthew Wilcox
@ 2019-04-03 16:40 ` Matteo Croce
2019-04-03 17:08 ` Matteo Croce
0 siblings, 1 reply; 11+ messages in thread
From: Matteo Croce @ 2019-04-03 16:40 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Christian Brauner, linux-fsdevel, LKML, Luis Chamberlain,
Kees Cook, Zev Weiss, Andrew Morton
On Wed, Apr 3, 2019 at 5:51 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Apr 03, 2019 at 05:24:26PM +0200, Matteo Croce wrote:
> > On Wed, Apr 3, 2019 at 4:02 PM Christian Brauner <christian@brauner.io> wrote:
> > > Yeah, maybe but it still feels cleaner and more obvious to just add:
> > >
> > > static long long_zero;
> > >
> > > given that most callers actually seem to want an (unsigned) int.
> > >
> > > I don't have a strong opinion though so if others feel that it's just a
> > > waste of space consider it acked.
> > >
> >
> > Well, given that the value is zero, in this expectional case we could
> > avoid duplicating the symbol and save 4 bytes.
> > What the maintainers think?
>
> If we care about saving four bytes, we could just pass the address of
> ZERO_PAGE(0).
That would work, work too, maybe it's a bit overkill.
int zero is always there and it's static, so enlarging it to long
should be a straightforward fix.
Obviously we can't do it for other numbers, but we can alias it just
for the zero case..
Regards,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
2019-04-03 16:40 ` Matteo Croce
@ 2019-04-03 17:08 ` Matteo Croce
2019-04-04 14:09 ` Christian Brauner
0 siblings, 1 reply; 11+ messages in thread
From: Matteo Croce @ 2019-04-03 17:08 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Christian Brauner, linux-fsdevel, LKML, Luis Chamberlain,
Kees Cook, Zev Weiss, Andrew Morton
On Wed, Apr 3, 2019 at 6:40 PM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Wed, Apr 3, 2019 at 5:51 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Apr 03, 2019 at 05:24:26PM +0200, Matteo Croce wrote:
> > > On Wed, Apr 3, 2019 at 4:02 PM Christian Brauner <christian@brauner.io> wrote:
> > > > Yeah, maybe but it still feels cleaner and more obvious to just add:
> > > >
> > > > static long long_zero;
> > > >
> > > > given that most callers actually seem to want an (unsigned) int.
> > > >
> > > > I don't have a strong opinion though so if others feel that it's just a
> > > > waste of space consider it acked.
> > > >
> > >
> > > Well, given that the value is zero, in this expectional case we could
> > > avoid duplicating the symbol and save 4 bytes.
> > > What the maintainers think?
> >
> > If we care about saving four bytes, we could just pass the address of
> > ZERO_PAGE(0).
>
> That would work, work too, maybe it's a bit overkill.
> int zero is always there and it's static, so enlarging it to long
> should be a straightforward fix.
> Obviously we can't do it for other numbers, but we can alias it just
> for the zero case..
>
> Regards,
>
> --
> Matteo Croce
> per aspera ad upstream
Anyway, I'm fine with both solutions, as I have other patches in the
queue which depends on this fix.
Regards,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
2019-03-28 13:03 [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max Matteo Croce
2019-04-03 11:54 ` Matteo Croce
2019-04-03 14:02 ` Christian Brauner
@ 2019-04-03 17:41 ` Kees Cook
2019-04-04 0:13 ` Matteo Croce
2 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2019-04-03 17:41 UTC (permalink / raw)
To: Matteo Croce; +Cc: linux-fsdevel, LKML, Luis Chamberlain
On Thu, Mar 28, 2019 at 6:03 AM Matteo Croce <mcroce@redhat.com> wrote:
>
> fs.file-max sysctl uses proc_doulongvec_minmax() as proc handler, which
> accesses *extra1 and *extra2 as unsigned long, but commit 32a5ad9c2285
> ("sysctl: handle overflow for file-max") assigns &zero, which is an int,
> to extra1, generating the following KASAN report.
> Fix this by changing 'zero' to long, which does not need to be duplicated
> like 'one' and 'one_ul' for two data types.
>
> ==================================================================
> BUG: KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax+0x2f9/0x600
> Read of size 8 at addr ffffffff8233dc20 by task systemd/1
>
> CPU: 0 PID: 1 Comm: systemd Not tainted 5.1.0-rc2-kvm+ #22
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS ?-20180724_192412-buildhw-07.phx2.fedoraproject.org-1.fc29 04/01/2014
> Call Trace:
> print_address_description+0x67/0x23d
> kasan_report.cold.3+0x1c/0x36
> __do_proc_doulongvec_minmax+0x2f9/0x600
> proc_doulongvec_minmax+0x3a/0x50
> proc_sys_call_handler+0x11d/0x170
> vfs_write+0xd7/0x200
> ksys_write+0x93/0x110
> do_syscall_64+0x57/0x140
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7f67d33e8804
> Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 5e 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53
> RSP: 002b:00007fffd9992ed8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f67d33e8804
> RDX: 0000000000000015 RSI: 00005586ce2607b0 RDI: 0000000000000004
> RBP: 00007fffd9992f30 R08: 000000000000c0c0 R09: ffffffffffff0000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
> R13: 0000000000000015 R14: 00005586ce2607c4 R15: 00007fffd9992f70
>
> The buggy address belongs to the variable:
> 0xffffffff8233dc20
>
> Memory state around the buggy address:
> ffffffff8233db00: 00 00 00 00 00 00 00 00 fa fa fa fa 04 fa fa fa
> ffffffff8233db80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa
> >ffffffff8233dc00: fa fa fa fa 04 fa fa fa fa fa fa fa 00 00 00 00
> ^
> ffffffff8233dc80: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
> ffffffff8233dd00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ==================================================================
>
> Fixes: 32a5ad9c2285 ("sysctl: handle overflow for file-max")
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---
> kernel/sysctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index e5da394d1ca3..3e959d67d619 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -124,7 +124,7 @@ static int sixty = 60;
>
> static int __maybe_unused neg_one = -1;
>
> -static int zero;
> +static long zero;
> static int __maybe_unused one = 1;
> static int __maybe_unused two = 2;
> static int __maybe_unused four = 4;
This seems okay to me; thanks for the fix! (I think it's fine to keep
this merged instead of a distinct long_zero, as long as we're not
seeing type warnings during the build.)
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
2019-04-03 17:41 ` Kees Cook
@ 2019-04-04 0:13 ` Matteo Croce
0 siblings, 0 replies; 11+ messages in thread
From: Matteo Croce @ 2019-04-04 0:13 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-fsdevel, LKML, Luis Chamberlain
On Wed, Apr 3, 2019 at 7:41 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Mar 28, 2019 at 6:03 AM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > fs.file-max sysctl uses proc_doulongvec_minmax() as proc handler, which
> > accesses *extra1 and *extra2 as unsigned long, but commit 32a5ad9c2285
> > ("sysctl: handle overflow for file-max") assigns &zero, which is an int,
> > to extra1, generating the following KASAN report.
> > Fix this by changing 'zero' to long, which does not need to be duplicated
> > like 'one' and 'one_ul' for two data types.
> >
> > ==================================================================
> > BUG: KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax+0x2f9/0x600
> > Read of size 8 at addr ffffffff8233dc20 by task systemd/1
> >
> > CPU: 0 PID: 1 Comm: systemd Not tainted 5.1.0-rc2-kvm+ #22
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS ?-20180724_192412-buildhw-07.phx2.fedoraproject.org-1.fc29 04/01/2014
> > Call Trace:
> > print_address_description+0x67/0x23d
> > kasan_report.cold.3+0x1c/0x36
> > __do_proc_doulongvec_minmax+0x2f9/0x600
> > proc_doulongvec_minmax+0x3a/0x50
> > proc_sys_call_handler+0x11d/0x170
> > vfs_write+0xd7/0x200
> > ksys_write+0x93/0x110
> > do_syscall_64+0x57/0x140
> > entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > RIP: 0033:0x7f67d33e8804
> > Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 5e 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53
> > RSP: 002b:00007fffd9992ed8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f67d33e8804
> > RDX: 0000000000000015 RSI: 00005586ce2607b0 RDI: 0000000000000004
> > RBP: 00007fffd9992f30 R08: 000000000000c0c0 R09: ffffffffffff0000
> > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
> > R13: 0000000000000015 R14: 00005586ce2607c4 R15: 00007fffd9992f70
> >
> > The buggy address belongs to the variable:
> > 0xffffffff8233dc20
> >
> > Memory state around the buggy address:
> > ffffffff8233db00: 00 00 00 00 00 00 00 00 fa fa fa fa 04 fa fa fa
> > ffffffff8233db80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa
> > >ffffffff8233dc00: fa fa fa fa 04 fa fa fa fa fa fa fa 00 00 00 00
> > ^
> > ffffffff8233dc80: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
> > ffffffff8233dd00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > ==================================================================
> >
> > Fixes: 32a5ad9c2285 ("sysctl: handle overflow for file-max")
> > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> > ---
> > kernel/sysctl.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index e5da394d1ca3..3e959d67d619 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -124,7 +124,7 @@ static int sixty = 60;
> >
> > static int __maybe_unused neg_one = -1;
> >
> > -static int zero;
> > +static long zero;
> > static int __maybe_unused one = 1;
> > static int __maybe_unused two = 2;
> > static int __maybe_unused four = 4;
>
> This seems okay to me; thanks for the fix! (I think it's fine to keep
> this merged instead of a distinct long_zero, as long as we're not
> seeing type warnings during the build.)
>
> Acked-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> --
> Kees Cook
No warnings with gcc version 8.3.1 20190223
CC kernel/sysctl.o
AR kernel/built-in.a
...
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
2019-04-03 17:08 ` Matteo Croce
@ 2019-04-04 14:09 ` Christian Brauner
2019-04-04 14:49 ` Matteo Croce
0 siblings, 1 reply; 11+ messages in thread
From: Christian Brauner @ 2019-04-04 14:09 UTC (permalink / raw)
To: Matteo Croce
Cc: Matthew Wilcox, linux-fsdevel, LKML, Luis Chamberlain, Kees Cook,
Zev Weiss, Andrew Morton
On Wed, Apr 03, 2019 at 07:08:47PM +0200, Matteo Croce wrote:
> On Wed, Apr 3, 2019 at 6:40 PM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > On Wed, Apr 3, 2019 at 5:51 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Wed, Apr 03, 2019 at 05:24:26PM +0200, Matteo Croce wrote:
> > > > On Wed, Apr 3, 2019 at 4:02 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > Yeah, maybe but it still feels cleaner and more obvious to just add:
> > > > >
> > > > > static long long_zero;
> > > > >
> > > > > given that most callers actually seem to want an (unsigned) int.
> > > > >
> > > > > I don't have a strong opinion though so if others feel that it's just a
> > > > > waste of space consider it acked.
> > > > >
> > > >
> > > > Well, given that the value is zero, in this expectional case we could
> > > > avoid duplicating the symbol and save 4 bytes.
> > > > What the maintainers think?
> > >
> > > If we care about saving four bytes, we could just pass the address of
> > > ZERO_PAGE(0).
> >
> > That would work, work too, maybe it's a bit overkill.
> > int zero is always there and it's static, so enlarging it to long
> > should be a straightforward fix.
> > Obviously we can't do it for other numbers, but we can alias it just
> > for the zero case..
> >
> > Regards,
> >
> > --
> > Matteo Croce
> > per aspera ad upstream
>
> Anyway, I'm fine with both solutions, as I have other patches in the
I think Matthew's idea gets us best of both worlds so I'd suggest to use
it and resend the patch. You likely want to Cc stable@vger.kernel.org
since the original patch this fixes got backported by Greg quite a bit
since this was a rather long-standing issue. Please also Cc Andrew this
time since he's likely going to pick it up.
Thanks for the patch!
Christian
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
2019-04-04 14:09 ` Christian Brauner
@ 2019-04-04 14:49 ` Matteo Croce
0 siblings, 0 replies; 11+ messages in thread
From: Matteo Croce @ 2019-04-04 14:49 UTC (permalink / raw)
To: Christian Brauner
Cc: Matthew Wilcox, linux-fsdevel, LKML, Luis Chamberlain, Kees Cook,
Zev Weiss, Andrew Morton, stable
On Thu, Apr 4, 2019 at 4:09 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Wed, Apr 03, 2019 at 07:08:47PM +0200, Matteo Croce wrote:
> > On Wed, Apr 3, 2019 at 6:40 PM Matteo Croce <mcroce@redhat.com> wrote:
> > >
> > > On Wed, Apr 3, 2019 at 5:51 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Wed, Apr 03, 2019 at 05:24:26PM +0200, Matteo Croce wrote:
> > > > > On Wed, Apr 3, 2019 at 4:02 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > Yeah, maybe but it still feels cleaner and more obvious to just add:
> > > > > >
> > > > > > static long long_zero;
> > > > > >
> > > > > > given that most callers actually seem to want an (unsigned) int.
> > > > > >
> > > > > > I don't have a strong opinion though so if others feel that it's just a
> > > > > > waste of space consider it acked.
> > > > > >
> > > > >
> > > > > Well, given that the value is zero, in this expectional case we could
> > > > > avoid duplicating the symbol and save 4 bytes.
> > > > > What the maintainers think?
> > > >
> > > > If we care about saving four bytes, we could just pass the address of
> > > > ZERO_PAGE(0).
> > >
> > > That would work, work too, maybe it's a bit overkill.
> > > int zero is always there and it's static, so enlarging it to long
> > > should be a straightforward fix.
> > > Obviously we can't do it for other numbers, but we can alias it just
> > > for the zero case..
> > >
> > > Regards,
> > >
> > > --
> > > Matteo Croce
> > > per aspera ad upstream
> >
> > Anyway, I'm fine with both solutions, as I have other patches in the
>
> I think Matthew's idea gets us best of both worlds so I'd suggest to use
> it and resend the patch. You likely want to Cc stable@vger.kernel.org
> since the original patch this fixes got backported by Greg quite a bit
> since this was a rather long-standing issue. Please also Cc Andrew this
> time since he's likely going to pick it up.
>
> Thanks for the patch!
> Christian
So you mean using page_address(ZERO_PAGE(0)) ?
The idea is nice, but since struct ctl_table kern_table is declared as
global variable, how can I assign it to the structure?
GCC complains about 'initializer element is not constant', and
ZERO_PAGE(0)->virtual only works if WANT_PAGE_VIRTUAL.
Anyway, I'm preparing a treewide patch to move all "zero", "one" and
"int_max" to three single, const variables in fs/proc/proc_sysctl.c
as there are 200+ occourrences of them, so I'd rather keep this simple
to have it easily backported to stable.
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-04-04 14:49 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-28 13:03 [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max Matteo Croce
2019-04-03 11:54 ` Matteo Croce
2019-04-03 14:02 ` Christian Brauner
2019-04-03 15:24 ` Matteo Croce
2019-04-03 15:51 ` Matthew Wilcox
2019-04-03 16:40 ` Matteo Croce
2019-04-03 17:08 ` Matteo Croce
2019-04-04 14:09 ` Christian Brauner
2019-04-04 14:49 ` Matteo Croce
2019-04-03 17:41 ` Kees Cook
2019-04-04 0:13 ` Matteo Croce
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).