linux-kernel - Re: [PATCH] FS: Fixed buffer overflow issue in seq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFyE=EDqMt1E94FSOFNEVWMS+mNVsJZYwiRmWfyB5HkykA@mail.gmail.com>
Date:	Mon, 18 Nov 2013 16:38:03 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	"Charley (Hao Chuan) Chu" <charley.chu@...adcom.com>
Cc:	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] FS: Fixed buffer overflow issue in seq_read()

Hmm.. Al - this looks like a major oversight, but it also looks like
the wrong place to initialize count/from in, just because it doesn't
follow any sane patterns.

My gut feel is that this needs more cleanup and some sane helper
function that always initializes those fields when allocating a new
buffer. Rather than the "initialize in random places and then miss a
few".

Afaik, those fields currently get (re-)initialized when:

 - We do the memset() of the whole seq_file structure at seq_open() time.

 - at the top of traverse()

 - count (but not from) gets reinitialized when growing the buffer or
after traverse() fails in seq_read()

and it really doesn't give me that happy fuzzy feeling of "that all
makes sense". Charley's patch seems to fix a missing initialization,
but I'd *really* like to have it all make more sense, and feel that
we're not missing some *other* initialization.

Al?

                Linus

On Mon, Nov 18, 2013 at 4:18 PM, Charley (Hao Chuan) Chu
<charley.chu@...adcom.com> wrote:
> The buffer count is not initialized when a new buffer is allocated.
>
> It cause kernel crash with "Unable to handle kernel paging
> request..." error in __copy_to_user_std(). It happens when a
> memory allocation failure in the while(1)-loop, which left the
> buffer count (m->count) is larger than buffer size
> (m->size).
>
> This patch is currently against a linux 3.12 kernel
>
> Signed-off-by: Charley Chu charley.chu@...adcom.com
> ---
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 1cd2388..480a341 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -191,6 +191,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
>
>         /* grab buffer if we didn't have one */
>         if (!m->buf) {
> +               m->count = m->from = 0;
>                 m->buf = kmalloc(m->size = PAGE_SIZE, GFP_KERNEL);
>                 if (!m->buf)
>                         goto Enomem;
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/