From: Christian Brauner <christian@brauner.io>
To: Matteo Croce <mcroce@redhat.com>
Cc: linux-fsdevel@vger.kernel.org,
LKML <linux-kernel@vger.kernel.org>,
Luis Chamberlain <mcgrof@kernel.org>,
Kees Cook <keescook@chromium.org>,
willy@infradead.org, zev@bewilderbeest.net,
akpm@linux-foundation.org
Subject: Re: [PATCH] kernel/sysctl.c: fix out of bounds access in fs.file-max
Date: Wed, 3 Apr 2019 16:02:04 +0200 [thread overview]
Message-ID: <20190403140203.qq37rgcikvoawb5f@brauner.io> (raw)
In-Reply-To: <20190328130306.25384-1-mcroce@redhat.com>
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
>
next prev parent reply other threads:[~2019-04-03 14:02 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190403140203.qq37rgcikvoawb5f@brauner.io \
--to=christian@brauner.io \
--cc=akpm@linux-foundation.org \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=mcroce@redhat.com \
--cc=willy@infradead.org \
--cc=zev@bewilderbeest.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).