[PATCH v2] isofs compress: Remove VLA usage

[PATCH v2] isofs compress: Remove VLA usage

[Kyle Spiers]

As part of the effort to remove VLAs from the kernel[1], this changes
the allocation of the bhs and pages arrays from being on the stack to being
kcalloc()ed. This also allows for the removal of the explicit zeroing
of bhs.

https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kyle Spiers <ksspiers@google.com>
---
 fs/isofs/compress.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
index 9bb2fe35799d..4eba16bf173c 100644
--- a/fs/isofs/compress.c
+++ b/fs/isofs/compress.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/bio.h>
 
+#include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/zlib.h>
 
@@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
 				>> bufshift;
 	int haveblocks;
 	blkcnt_t blocknum;
-	struct buffer_head *bhs[needblocks + 1];
+	struct buffer_head **bhs;
 	int curbh, curpage;
 
 	if (block_size > deflateBound(1UL << zisofs_block_shift)) {
@@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
 
 	/* Because zlib is not thread-safe, do all the I/O at the top. */
 	blocknum = block_start >> bufshift;
-	memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
+	bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
+	if (!bhs)
+		return -ENOMEM;
 	haveblocks = isofs_get_blocks(inode, blocknum, bhs, needblocks);
 	ll_rw_block(REQ_OP_READ, 0, haveblocks, bhs);
 
@@ -190,6 +193,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
 b_eio:
 	for (i = 0; i < haveblocks; i++)
 		brelse(bhs[i]);
+	kfree(bhs);
 	return stream.total_out;
 }
 
@@ -305,7 +309,7 @@ static int zisofs_readpage(struct file *file, struct page *page)
 	unsigned int zisofs_pages_per_cblock =
 		PAGE_SHIFT <= zisofs_block_shift ?
 		(1 << (zisofs_block_shift - PAGE_SHIFT)) : 0;
-	struct page *pages[max_t(unsigned, zisofs_pages_per_cblock, 1)];
+	struct page **pages;
 	pgoff_t index = page->index, end_index;
 
 	end_index = (inode->i_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -330,6 +334,10 @@ static int zisofs_readpage(struct file *file, struct page *page)
 		full_page = 0;
 		pcount = 1;
 	}
+	pages = kcalloc(max_t(unsigned int, zisofs_pages_per_cblock, 1),
+					sizeof(*pages), GFP_KERNEL);
+	if (!pages)
+		return -ENOMEM;
 	pages[full_page] = page;
 
 	for (i = 0; i < pcount; i++, index++) {
@@ -357,6 +365,7 @@ static int zisofs_readpage(struct file *file, struct page *page)
 	}			
 
 	/* At this point, err contains 0 or -EIO depending on the "critical" page */
+	kfree(pages);
 	return err;
 }
 
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH v2] isofs compress: Remove VLA usage

[Kees Cook]

On Thu, Apr 5, 2018 at 11:17 AM, Kyle Spiers <ksspiers@google.com> wrote:
> As part of the effort to remove VLAs from the kernel[1], this changes
> the allocation of the bhs and pages arrays from being on the stack to being
> kcalloc()ed. This also allows for the removal of the explicit zeroing
> of bhs.
>
> https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Kyle Spiers <ksspiers@google.com>

Looks good! Thanks for fixing the sparc64 build failure. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  fs/isofs/compress.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> index 9bb2fe35799d..4eba16bf173c 100644
> --- a/fs/isofs/compress.c
> +++ b/fs/isofs/compress.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/bio.h>
>
> +#include <linux/slab.h>
>  #include <linux/vmalloc.h>
>  #include <linux/zlib.h>
>
> @@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
>                                 >> bufshift;
>         int haveblocks;
>         blkcnt_t blocknum;
> -       struct buffer_head *bhs[needblocks + 1];
> +       struct buffer_head **bhs;
>         int curbh, curpage;
>
>         if (block_size > deflateBound(1UL << zisofs_block_shift)) {
> @@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
>
>         /* Because zlib is not thread-safe, do all the I/O at the top. */
>         blocknum = block_start >> bufshift;
> -       memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
> +       bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
> +       if (!bhs)
> +               return -ENOMEM;
>         haveblocks = isofs_get_blocks(inode, blocknum, bhs, needblocks);
>         ll_rw_block(REQ_OP_READ, 0, haveblocks, bhs);
>
> @@ -190,6 +193,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
>  b_eio:
>         for (i = 0; i < haveblocks; i++)
>                 brelse(bhs[i]);
> +       kfree(bhs);
>         return stream.total_out;
>  }
>
> @@ -305,7 +309,7 @@ static int zisofs_readpage(struct file *file, struct page *page)
>         unsigned int zisofs_pages_per_cblock =
>                 PAGE_SHIFT <= zisofs_block_shift ?
>                 (1 << (zisofs_block_shift - PAGE_SHIFT)) : 0;
> -       struct page *pages[max_t(unsigned, zisofs_pages_per_cblock, 1)];
> +       struct page **pages;
>         pgoff_t index = page->index, end_index;
>
>         end_index = (inode->i_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> @@ -330,6 +334,10 @@ static int zisofs_readpage(struct file *file, struct page *page)
>                 full_page = 0;
>                 pcount = 1;
>         }
> +       pages = kcalloc(max_t(unsigned int, zisofs_pages_per_cblock, 1),
> +                                       sizeof(*pages), GFP_KERNEL);
> +       if (!pages)
> +               return -ENOMEM;
>         pages[full_page] = page;
>
>         for (i = 0; i < pcount; i++, index++) {
> @@ -357,6 +365,7 @@ static int zisofs_readpage(struct file *file, struct page *page)
>         }
>
>         /* At this point, err contains 0 or -EIO depending on the "critical" page */
> +       kfree(pages);
>         return err;
>  }
>
> --
> 2.17.0.484.g0c8726318c-goog
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v2] isofs compress: Remove VLA usage

[Joe Perches]

On Thu, 2018-04-05 at 11:17 -0700, Kyle Spiers wrote:
> As part of the effort to remove VLAs from the kernel[1], this changes
> the allocation of the bhs and pages arrays from being on the stack to being
> kcalloc()ed. This also allows for the removal of the explicit zeroing
> of bhs.
> 
> https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kyle Spiers <ksspiers@google.com>
> ---
>  fs/isofs/compress.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> index 9bb2fe35799d..4eba16bf173c 100644
> --- a/fs/isofs/compress.c
> +++ b/fs/isofs/compress.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/bio.h>
>  
> +#include <linux/slab.h>
>  #include <linux/vmalloc.h>
>  #include <linux/zlib.h>
>  
> @@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
>  				>> bufshift;
>  	int haveblocks;
>  	blkcnt_t blocknum;
> -	struct buffer_head *bhs[needblocks + 1];
> +	struct buffer_head **bhs;
>  	int curbh, curpage;
>  
>  	if (block_size > deflateBound(1UL << zisofs_block_shift)) {
> @@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
>  
>  	/* Because zlib is not thread-safe, do all the I/O at the top. */
>  	blocknum = block_start >> bufshift;
> -	memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
> +	bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
> +	if (!bhs)
> +		return -ENOMEM;

This direct return appears incorrect.

It seems it should be:

	if (!bhs) {
		*errp = -ENOMEM;
		return 0;
	}

Re: [PATCH v2] isofs compress: Remove VLA usage

[Jan Kara]

On Thu 05-04-18 11:17:20, Kyle Spiers wrote:
> As part of the effort to remove VLAs from the kernel[1], this changes
> the allocation of the bhs and pages arrays from being on the stack to being
> kcalloc()ed. This also allows for the removal of the explicit zeroing
> of bhs.
> 
> https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kyle Spiers <ksspiers@google.com>

This is a good cleanup but the error recovery is hosed. See below...

> ---
>  fs/isofs/compress.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/isofs/compress.c b/fs/isofs/compress.c
> index 9bb2fe35799d..4eba16bf173c 100644
> --- a/fs/isofs/compress.c
> +++ b/fs/isofs/compress.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/bio.h>
>  
> +#include <linux/slab.h>
>  #include <linux/vmalloc.h>
>  #include <linux/zlib.h>
>  
> @@ -59,7 +60,7 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
>  				>> bufshift;
>  	int haveblocks;
>  	blkcnt_t blocknum;
> -	struct buffer_head *bhs[needblocks + 1];
> +	struct buffer_head **bhs;
>  	int curbh, curpage;
>  
>  	if (block_size > deflateBound(1UL << zisofs_block_shift)) {
> @@ -80,7 +81,9 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
>  
>  	/* Because zlib is not thread-safe, do all the I/O at the top. */
>  	blocknum = block_start >> bufshift;
> -	memset(bhs, 0, (needblocks + 1) * sizeof(struct buffer_head *));
> +	bhs = kcalloc(needblocks + 1, sizeof(*bhs), GFP_KERNEL);
> +	if (!bhs)
> +		return -ENOMEM;

As Joe pointed out this needs to be:

	if (!bhs) {
		*errp = -ENOMEM;
		return 0;
	}

> @@ -330,6 +334,10 @@ static int zisofs_readpage(struct file *file, struct page *page)
>  		full_page = 0;
>  		pcount = 1;
>  	}
> +	pages = kcalloc(max_t(unsigned int, zisofs_pages_per_cblock, 1),
> +					sizeof(*pages), GFP_KERNEL);
> +	if (!pages)
> +		return -ENOMEM;

And this is wrong as well. You need to do:

	if (!pages) {
		unlock_page(page);
		return -ENOMEM;
	}


								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR