linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] seq_file: Unconditionally use vmalloc for buffer
@ 2021-03-12 20:55 Kees Cook
  2021-03-13  8:54 ` Greg Kroah-Hartman
  2021-03-15  8:34 ` Michal Hocko
  0 siblings, 2 replies; 4+ messages in thread
From: Kees Cook @ 2021-03-12 20:55 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Greg Kroah-Hartman, Michal Hocko, Alexey Dobriyan,
	Lee Duncan, Chris Leech, Adam Nichols, linux-kernel,
	linux-fsdevel, linux-hardening

The sysfs interface to seq_file continues to be rather fragile, as seen
with some recent exploits[1]. Move the seq_file buffer to the vmap area
(while retaining the accounting flag), since it has guard pages that
will catch and stop linear overflows. This seems justified given that
seq_file already uses kvmalloc(), that allocations are normally short
lived, and that they are not normally performance critical.

[1] https://blog.grimm-co.com/2021/03/new-old-bugs-in-linux-kernel.html

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/seq_file.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index cb11a34fb871..ad78577d4c2c 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -32,7 +32,7 @@ static void seq_set_overflow(struct seq_file *m)
 
 static void *seq_buf_alloc(unsigned long size)
 {
-	return kvmalloc(size, GFP_KERNEL_ACCOUNT);
+	return __vmalloc(size, GFP_KERNEL_ACCOUNT);
 }
 
 /**
@@ -130,7 +130,7 @@ static int traverse(struct seq_file *m, loff_t offset)
 
 Eoverflow:
 	m->op->stop(m, p);
-	kvfree(m->buf);
+	vfree(m->buf);
 	m->count = 0;
 	m->buf = seq_buf_alloc(m->size <<= 1);
 	return !m->buf ? -ENOMEM : -EAGAIN;
@@ -237,7 +237,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 			goto Fill;
 		// need a bigger buffer
 		m->op->stop(m, p);
-		kvfree(m->buf);
+		vfree(m->buf);
 		m->count = 0;
 		m->buf = seq_buf_alloc(m->size <<= 1);
 		if (!m->buf)
@@ -349,7 +349,7 @@ EXPORT_SYMBOL(seq_lseek);
 int seq_release(struct inode *inode, struct file *file)
 {
 	struct seq_file *m = file->private_data;
-	kvfree(m->buf);
+	vfree(m->buf);
 	kmem_cache_free(seq_file_cache, m);
 	return 0;
 }
@@ -585,7 +585,7 @@ int single_open_size(struct file *file, int (*show)(struct seq_file *, void *),
 		return -ENOMEM;
 	ret = single_open(file, show, data);
 	if (ret) {
-		kvfree(buf);
+		vfree(buf);
 		return ret;
 	}
 	((struct seq_file *)file->private_data)->buf = buf;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] seq_file: Unconditionally use vmalloc for buffer
  2021-03-12 20:55 [PATCH] seq_file: Unconditionally use vmalloc for buffer Kees Cook
@ 2021-03-13  8:54 ` Greg Kroah-Hartman
  2021-03-15  8:34 ` Michal Hocko
  1 sibling, 0 replies; 4+ messages in thread
From: Greg Kroah-Hartman @ 2021-03-13  8:54 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, Michal Hocko, Alexey Dobriyan, Lee Duncan,
	Chris Leech, Adam Nichols, linux-kernel, linux-fsdevel,
	linux-hardening

On Fri, Mar 12, 2021 at 12:55:58PM -0800, Kees Cook wrote:
> The sysfs interface to seq_file continues to be rather fragile, as seen
> with some recent exploits[1]. Move the seq_file buffer to the vmap area
> (while retaining the accounting flag), since it has guard pages that
> will catch and stop linear overflows. This seems justified given that
> seq_file already uses kvmalloc(), that allocations are normally short
> lived, and that they are not normally performance critical.
> 
> [1] https://blog.grimm-co.com/2021/03/new-old-bugs-in-linux-kernel.html
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/seq_file.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index cb11a34fb871..ad78577d4c2c 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -32,7 +32,7 @@ static void seq_set_overflow(struct seq_file *m)
>  
>  static void *seq_buf_alloc(unsigned long size)
>  {
> -	return kvmalloc(size, GFP_KERNEL_ACCOUNT);
> +	return __vmalloc(size, GFP_KERNEL_ACCOUNT);

Maybe a small comment here like:
	/* use vmalloc as it has good bounds checking */
so we know why this is being used instead of kmalloc() or anything else?

Other than that, no objection from me:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] seq_file: Unconditionally use vmalloc for buffer
  2021-03-12 20:55 [PATCH] seq_file: Unconditionally use vmalloc for buffer Kees Cook
  2021-03-13  8:54 ` Greg Kroah-Hartman
@ 2021-03-15  8:34 ` Michal Hocko
  2021-03-15 17:40   ` Kees Cook
  1 sibling, 1 reply; 4+ messages in thread
From: Michal Hocko @ 2021-03-15  8:34 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, Greg Kroah-Hartman, Alexey Dobriyan, Lee Duncan,
	Chris Leech, Adam Nichols, linux-kernel, linux-fsdevel,
	linux-hardening

On Fri 12-03-21 12:55:58, Kees Cook wrote:
> The sysfs interface to seq_file continues to be rather fragile, as seen
> with some recent exploits[1]. Move the seq_file buffer to the vmap area
> (while retaining the accounting flag), since it has guard pages that
> will catch and stop linear overflows. This seems justified given that
> seq_file already uses kvmalloc(), that allocations are normally short
> lived, and that they are not normally performance critical.

What is the runtime effect of this change? The interface is widely used
for many other interfaces - e.g. in proc. While from the correctness POV
this should be OK (ish for 64b it is definitely problem for kernels with
lowmem and limited vmalloc space). Vmalloc is also to be expected to
regress in performance for small allocations which is the most usual
case.
 
> [1] https://blog.grimm-co.com/2021/03/new-old-bugs-in-linux-kernel.html
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/seq_file.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index cb11a34fb871..ad78577d4c2c 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -32,7 +32,7 @@ static void seq_set_overflow(struct seq_file *m)
>  
>  static void *seq_buf_alloc(unsigned long size)
>  {
> -	return kvmalloc(size, GFP_KERNEL_ACCOUNT);
> +	return __vmalloc(size, GFP_KERNEL_ACCOUNT);
>  }
>  
>  /**
> @@ -130,7 +130,7 @@ static int traverse(struct seq_file *m, loff_t offset)
>  
>  Eoverflow:
>  	m->op->stop(m, p);
> -	kvfree(m->buf);
> +	vfree(m->buf);
>  	m->count = 0;
>  	m->buf = seq_buf_alloc(m->size <<= 1);
>  	return !m->buf ? -ENOMEM : -EAGAIN;
> @@ -237,7 +237,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  			goto Fill;
>  		// need a bigger buffer
>  		m->op->stop(m, p);
> -		kvfree(m->buf);
> +		vfree(m->buf);
>  		m->count = 0;
>  		m->buf = seq_buf_alloc(m->size <<= 1);
>  		if (!m->buf)
> @@ -349,7 +349,7 @@ EXPORT_SYMBOL(seq_lseek);
>  int seq_release(struct inode *inode, struct file *file)
>  {
>  	struct seq_file *m = file->private_data;
> -	kvfree(m->buf);
> +	vfree(m->buf);
>  	kmem_cache_free(seq_file_cache, m);
>  	return 0;
>  }
> @@ -585,7 +585,7 @@ int single_open_size(struct file *file, int (*show)(struct seq_file *, void *),
>  		return -ENOMEM;
>  	ret = single_open(file, show, data);
>  	if (ret) {
> -		kvfree(buf);
> +		vfree(buf);
>  		return ret;
>  	}
>  	((struct seq_file *)file->private_data)->buf = buf;
> -- 
> 2.25.1

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] seq_file: Unconditionally use vmalloc for buffer
  2021-03-15  8:34 ` Michal Hocko
@ 2021-03-15 17:40   ` Kees Cook
  0 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2021-03-15 17:40 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Greg Kroah-Hartman, Alexey Dobriyan, Lee Duncan,
	Chris Leech, Adam Nichols, linux-kernel, linux-fsdevel,
	linux-hardening

On Mon, Mar 15, 2021 at 09:34:18AM +0100, Michal Hocko wrote:
> On Fri 12-03-21 12:55:58, Kees Cook wrote:
> > The sysfs interface to seq_file continues to be rather fragile, as seen
> > with some recent exploits[1]. Move the seq_file buffer to the vmap area
> > (while retaining the accounting flag), since it has guard pages that
> > will catch and stop linear overflows. This seems justified given that
> > seq_file already uses kvmalloc(), that allocations are normally short
> > lived, and that they are not normally performance critical.
> 
> What is the runtime effect of this change? The interface is widely used

I haven't been able to measure any differences yet, but maybe I lack
imagination about workloads that are heavy on /sys or /proc accesses.

> for many other interfaces - e.g. in proc. While from the correctness POV
> this should be OK (ish for 64b it is definitely problem for kernels with
> lowmem and limited vmalloc space). Vmalloc is also to be expected to
> regress in performance for small allocations which is the most usual
> case.

seq_file's default size is PAGE_SIZE (and just goes up by powers of 2
from there), with the rare (3 callers) exception of single_open_size(),
which for at least 1 case is always >PAGE_SIZE. (I realize PAGE_SIZE may
be considered "small" for vmalloc, but I think gaining the guard page is
worth it, given the recurring flaws we see with at least sysfs handlers.)

-Kees

>  
> > [1] https://blog.grimm-co.com/2021/03/new-old-bugs-in-linux-kernel.html
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  fs/seq_file.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/seq_file.c b/fs/seq_file.c
> > index cb11a34fb871..ad78577d4c2c 100644
> > --- a/fs/seq_file.c
> > +++ b/fs/seq_file.c
> > @@ -32,7 +32,7 @@ static void seq_set_overflow(struct seq_file *m)
> >  
> >  static void *seq_buf_alloc(unsigned long size)
> >  {
> > -	return kvmalloc(size, GFP_KERNEL_ACCOUNT);
> > +	return __vmalloc(size, GFP_KERNEL_ACCOUNT);
> >  }
> >  
> >  /**
> > @@ -130,7 +130,7 @@ static int traverse(struct seq_file *m, loff_t offset)
> >  
> >  Eoverflow:
> >  	m->op->stop(m, p);
> > -	kvfree(m->buf);
> > +	vfree(m->buf);
> >  	m->count = 0;
> >  	m->buf = seq_buf_alloc(m->size <<= 1);
> >  	return !m->buf ? -ENOMEM : -EAGAIN;
> > @@ -237,7 +237,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> >  			goto Fill;
> >  		// need a bigger buffer
> >  		m->op->stop(m, p);
> > -		kvfree(m->buf);
> > +		vfree(m->buf);
> >  		m->count = 0;
> >  		m->buf = seq_buf_alloc(m->size <<= 1);
> >  		if (!m->buf)
> > @@ -349,7 +349,7 @@ EXPORT_SYMBOL(seq_lseek);
> >  int seq_release(struct inode *inode, struct file *file)
> >  {
> >  	struct seq_file *m = file->private_data;
> > -	kvfree(m->buf);
> > +	vfree(m->buf);
> >  	kmem_cache_free(seq_file_cache, m);
> >  	return 0;
> >  }
> > @@ -585,7 +585,7 @@ int single_open_size(struct file *file, int (*show)(struct seq_file *, void *),
> >  		return -ENOMEM;
> >  	ret = single_open(file, show, data);
> >  	if (ret) {
> > -		kvfree(buf);
> > +		vfree(buf);
> >  		return ret;
> >  	}
> >  	((struct seq_file *)file->private_data)->buf = buf;
> > -- 
> > 2.25.1
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-03-15 17:41 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12 20:55 [PATCH] seq_file: Unconditionally use vmalloc for buffer Kees Cook
2021-03-13  8:54 ` Greg Kroah-Hartman
2021-03-15  8:34 ` Michal Hocko
2021-03-15 17:40   ` Kees Cook

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