[PATCH] FS: Fixed buffer overflow issue in seq_read()

[PATCH] FS: Fixed buffer overflow issue in seq_read()

[Charley (Hao Chuan) Chu]

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@broadcom.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;

Re: [PATCH] FS: Fixed buffer overflow issue in seq_read()

[Linus Torvalds]

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@broadcom.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@broadcom.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;
>
>
>

Re: [PATCH] FS: Fixed buffer overflow issue in seq_read()

[Al Viro]

On Mon, Nov 18, 2013 at 04:38:03PM -0800, Linus Torvalds wrote:
> 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?

See upthread.  The bug is real, but I would rather go for a different
fix; it's not worth helper functions, though - we have exactly two places
where free m->buf without freeing m itself, and all we need to do is
clearing m->count in those two places.  No point delaying that to the
next call of seq_read() (and no point cleaning m->from at all), as soon
as we free m->buf we obviously lose all the data that might've been in it.

Re: [PATCH] FS: Fixed buffer overflow issue in seq_read()

[Al Viro]

On Tue, Nov 19, 2013 at 12:18:41AM +0000, Charley (Hao Chuan) Chu 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

The bug is real, but I don't like the fix.  First of all, m->from is
a red herring - it's not even looked at if m->count is 0.  The real
bug is that m->count is not cleared when m->buf is freed - at those
points we have zero bytes of valid contents reachable via m->buf.

What's more, one of those two points (in seq_read()) has cleaning of
->count right after successful kmalloc() following that kfree(), so
it just needs to be moved up a bit.  The other one (in traverse())
is missing.  So how about this:

seq_file: always clear m->count when we free m->buf

Once we'd freed m->buf, m->count should become zero - we have no
valid contents reachable via m->buf.

Reported-by: Charley (Hao Chuan) Chu <charley.chu@broadcom.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1cd2388..1d641bb 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -136,6 +136,7 @@ static int traverse(struct seq_file *m, loff_t offset)
 Eoverflow:
 	m->op->stop(m, p);
 	kfree(m->buf);
+	m->count = 0;
 	m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
 	return !m->buf ? -ENOMEM : -EAGAIN;
 }
@@ -232,10 +233,10 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 			goto Fill;
 		m->op->stop(m, p);
 		kfree(m->buf);
+		m->count = 0;
 		m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
 		if (!m->buf)
 			goto Enomem;
-		m->count = 0;
 		m->version = 0;
 		pos = m->index;
 		p = m->op->start(m, &pos);

Re: [PATCH] FS: Fixed buffer overflow issue in seq_read()

[Linus Torvalds]

On Tue, 19 Nov 2013, Al Viro wrote:
> 
> seq_file: always clear m->count when we free m->buf

Ok, applied.

What do you think about then just abstracing out that now common sequence 
of re-allocating a larger buffer, while clearing m->count?

IOW, something like the appended..

              Linus

---
 fs/seq_file.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1d641bb108d2..f513e1efa49d 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -82,6 +82,14 @@ int seq_open(struct file *file, const struct seq_operations *op)
 }
 EXPORT_SYMBOL(seq_open);
 
+static int grow_seq_buf(struct seq_file *m)
+{
+	kfree(m->buf);
+	m->count = 0;
+	m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
+	return m->buf != NULL;
+}
+
 static int traverse(struct seq_file *m, loff_t offset)
 {
 	loff_t pos = 0, index;
@@ -135,10 +143,7 @@ static int traverse(struct seq_file *m, loff_t offset)
 
 Eoverflow:
 	m->op->stop(m, p);
-	kfree(m->buf);
-	m->count = 0;
-	m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
-	return !m->buf ? -ENOMEM : -EAGAIN;
+	return grow_seq_buf(m) ? -EAGAIN : -ENOMEM;
 }
 
 /**
@@ -232,10 +237,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 		if (m->count < m->size)
 			goto Fill;
 		m->op->stop(m, p);
-		kfree(m->buf);
-		m->count = 0;
-		m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
-		if (!m->buf)
+		if (!grow_seq_buf(m))
 			goto Enomem;
 		m->version = 0;
 		pos = m->index;

Re: [PATCH] FS: Fixed buffer overflow issue in seq_read()

[Al Viro]

On Mon, Nov 18, 2013 at 07:13:54PM -0800, Linus Torvalds wrote:
> 
> 
> On Tue, 19 Nov 2013, Al Viro wrote:
> > 
> > seq_file: always clear m->count when we free m->buf
> 
> Ok, applied.
> 
> What do you think about then just abstracing out that now common sequence 
> of re-allocating a larger buffer, while clearing m->count?

Sure, no problem, but then we really have only 2 places doing that and no
visible cause to grow more of them.  With this common sequence being that
short, I'm not sure that effort to recall the definition of that helper
won't be more than that to understand the open-coded variant.  Matter of
taste, but IMO in this case the helper makes it slightly less readable...

BTW, I've several old commits that didn't go into the first pile (e.g.
taking read_seqbegin_or_lock() and friends from fs/dcache.c into
linux/seqlock.h, where they obviously belong, etc.) and several regression
fixes; are you OK with pull request tomorrow?  I can post it tonight,
but I'd prefer to leave local toruture running overnight...

Re: [PATCH] FS: Fixed buffer overflow issue in seq_read()

[Linus Torvalds]

On Mon, Nov 18, 2013 at 7:28 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> BTW, I've several old commits that didn't go into the first pile (e.g.
> taking read_seqbegin_or_lock() and friends from fs/dcache.c into
> linux/seqlock.h, where they obviously belong, etc.) and several regression
> fixes; are you OK with pull request tomorrow?  I can post it tonight,
> but I'd prefer to leave local toruture running overnight...

Tomorrow's fine.

           Linus

RE: [PATCH] FS: Fixed buffer overflow issue in seq_read()

[Charley (Hao Chuan) Chu]

> m->from is a red herring - it's not even looked at if m->count is 0.  

Then, shall the initialization here be removed too?

@@ -90,7 +90,7 @@ static int traverse(struct seq_file *m, loff_t offset)
 
        m->version = 0;
        index = 0;
-       m->count = m->from = 0;
+       m->count = 0;
        if (!offset) {
                m->index = index;
                return 0;

> What do you think about then just abstracing out that now common sequence 
> of re-allocating a larger buffer, while clearing m->count?

Following code is duplicated (slightly different) in both seq_read() and seq_lseek().
It would be nice to have them consolidated in traverse().

                while ((err = traverse(m, *ppos)) == -EAGAIN)
                        ;   
                if (err) {
                        /* With prejudice... */
                        m->read_pos = 0;
                        m->version = 0;
                        m->index = 0;
                        m->count = 0;
                        goto Done;
                } else {
                        m->read_pos = *ppos;
                }   

Thanks,
Charley