Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700943 > unrolled thread

[PATCH 1/2] ext4: fix warning about stack corruption

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] ext4: fix warning about stack corruption Arnd Bergmann <arnd@arndb.de> - 2017-08-01 14:10 +0200
    [PATCH 2/2] adfs: use 'unsigned' types for memcpy length Arnd Bergmann <arnd@arndb.de> - 2017-08-01 14:10 +0200
      Re: [PATCH 2/2] adfs: use 'unsigned' types for memcpy length Kees Cook <keescook@chromium.org> - 2017-08-01 20:30 +0200
        Re: [PATCH 2/2] adfs: use 'unsigned' types for memcpy length Stephen Rothwell <sfr@canb.auug.org.au> - 2017-08-01 23:50 +0200
    Re: [PATCH 1/2] ext4: fix warning about stack corruption Kees Cook <keescook@chromium.org> - 2017-08-01 20:30 +0200
    Re: [PATCH 1/2] ext4: fix warning about stack corruption Theodore Ts'o <tytso@mit.edu> - 2017-08-06 04:00 +0200
      Re: [PATCH 1/2] ext4: fix warning about stack corruption Arnd Bergmann <arnd@arndb.de> - 2017-08-06 22:40 +0200
    Re: [PATCH 1/2] ext4: fix warning about stack corruption Chandan Rajendra <chandan@linux.vnet.ibm.com> - 2017-08-07 08:50 +0200

#1700943 — [PATCH 1/2] ext4: fix warning about stack corruption

After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"),
we get a warning about possible stack overflow from a memcpy that
was not strictly bounded to the size of the local variable:

    inlined from 'ext4_mb_seq_groups_show' at fs/ext4/mballoc.c:2322:2:
include/linux/string.h:309:9: error: '__builtin_memcpy': writing between 161 and 1116 bytes into a region of size 160 overflows the destination [-Werror=stringop-overflow=]

We actually had a bug here that would have been found by the warning,
but it was already fixed last year in commit 30a9d7afe70e ("ext4: fix
stack memory corruption with 64k block size").

This replaces the fixed-length structure on the stack with a variable-length
structure, using the correct upper bound that tells the compiler that
everything is really fine here. I also change the loop count to check
for the same upper bound for consistency, but the existing code is
already correct here.

Note that while clang won't allow certain kinds of variable-length arrays
in structures, this particular instance is fine, as the array is at the
end of the structure, and the size is strictly bounded.

There is one remaining issue with the function that I'm not addressing
here: With s_blocksize_bits==16, we don't actually print the last two
members of the array, as we loop though just the first 14 members.
This could be easily addressed by adding two extra columns in the output,
but that could in theory break parsers in user space, and should be
a separate patch if we decide to modify it.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/ext4/mballoc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 581e357e8406..803cab1939fe 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2295,9 +2295,12 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
 	int err, buddy_loaded = 0;
 	struct ext4_buddy e4b;
 	struct ext4_group_info *grinfo;
+	unsigned char blocksize_bits = min_t(unsigned char,
+					     sb->s_blocksize_bits,
+					     EXT4_MAX_BLOCK_LOG_SIZE);
 	struct sg {
 		struct ext4_group_info info;
-		ext4_grpblk_t counters[EXT4_MAX_BLOCK_LOG_SIZE + 2];
+		ext4_grpblk_t counters[blocksize_bits + 2];
 	} sg;
 
 	group--;
@@ -2306,8 +2309,6 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
 			      " 2^0   2^1   2^2   2^3   2^4   2^5   2^6  "
 			      " 2^7   2^8   2^9   2^10  2^11  2^12  2^13  ]\n");
 
-	i = (sb->s_blocksize_bits + 2) * sizeof(sg.info.bb_counters[0]) +
-		sizeof(struct ext4_group_info);
 	grinfo = ext4_get_group_info(sb, group);
 	/* Load the group info in memory only if not already loaded. */
 	if (unlikely(EXT4_MB_GRP_NEED_INIT(grinfo))) {
@@ -2319,7 +2320,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
 		buddy_loaded = 1;
 	}
 
-	memcpy(&sg, ext4_get_group_info(sb, group), i);
+	memcpy(&sg, ext4_get_group_info(sb, group), sizeof(sg));
 
 	if (buddy_loaded)
 		ext4_mb_unload_buddy(&e4b);
@@ -2327,7 +2328,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
 	seq_printf(seq, "#%-5u: %-5u %-5u %-5u [", group, sg.info.bb_free,
 			sg.info.bb_fragments, sg.info.bb_first_free);
 	for (i = 0; i <= 13; i++)
-		seq_printf(seq, " %-5u", i <= sb->s_blocksize_bits + 1 ?
+		seq_printf(seq, " %-5u", i <= blocksize_bits + 1 ?
 				sg.info.bb_counters[i] : 0);
 	seq_printf(seq, " ]\n");
 
-- 
2.9.0

[toc] | [next] | [standalone]

#1700944 — [PATCH 2/2] adfs: use 'unsigned' types for memcpy length

After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we
get a warning in adfs about a possible buffer overflow:

In function 'memcpy',
    inlined from '__adfs_dir_put' at fs/adfs/dir_f.c:318:2,
    inlined from 'adfs_f_update' at fs/adfs/dir_f.c:403:2:
include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
    __read_overflow2();
    ^~~~~~~~~~~~~~~~~~

The warning is correct in the sense that a negative 'pos' argument
to the function would have that result. However, this is not a bug,
as we know the position is always positive (in fact, between 5
and 2007, inclusive) when the function gets called.

Changing the variable to a unsigned type avoids the problem. I decided
to use 'unsigned int' for the position in the directory and the block
number, as they are both counting things, but use size_t for the
offset and length that get passed into memcpy. This shuts up the
warning.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/adfs/dir_f.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/adfs/dir_f.c b/fs/adfs/dir_f.c
index 0fbfd0b04ae0..dab3595a1ecc 100644
--- a/fs/adfs/dir_f.c
+++ b/fs/adfs/dir_f.c
@@ -283,11 +283,12 @@ __adfs_dir_get(struct adfs_dir *dir, int pos, struct object_info *obj)
 }
 
 static int
-__adfs_dir_put(struct adfs_dir *dir, int pos, struct object_info *obj)
+__adfs_dir_put(struct adfs_dir *dir, unsigned int pos, struct object_info *obj)
 {
 	struct super_block *sb = dir->sb;
 	struct adfs_direntry de;
-	int thissize, buffer, offset;
+	unsigned int buffer;
+	size_t thissize, offset;
 
 	buffer = pos >> sb->s_blocksize_bits;
 
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1701328 — Re: [PATCH 2/2] adfs: use 'unsigned' types for memcpy length

On Tue, Aug 1, 2017 at 5:04 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we
> get a warning in adfs about a possible buffer overflow:
>
> In function 'memcpy',
>     inlined from '__adfs_dir_put' at fs/adfs/dir_f.c:318:2,
>     inlined from 'adfs_f_update' at fs/adfs/dir_f.c:403:2:
> include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
>     __read_overflow2();
>     ^~~~~~~~~~~~~~~~~~
>
> The warning is correct in the sense that a negative 'pos' argument
> to the function would have that result. However, this is not a bug,
> as we know the position is always positive (in fact, between 5
> and 2007, inclusive) when the function gets called.
>
> Changing the variable to a unsigned type avoids the problem. I decided
> to use 'unsigned int' for the position in the directory and the block
> number, as they are both counting things, but use size_t for the
> offset and length that get passed into memcpy. This shuts up the
> warning.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

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

Thanks for the fix! (Added sfr to Cc since he noticed this too.)

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1701526 — Re: [PATCH 2/2] adfs: use 'unsigned' types for memcpy length

Hi Arnd,

On Tue, 1 Aug 2017 11:20:26 -0700 Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Aug 1, 2017 at 5:04 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we
> > get a warning in adfs about a possible buffer overflow:
> >
> > In function 'memcpy',
> >     inlined from '__adfs_dir_put' at fs/adfs/dir_f.c:318:2,
> >     inlined from 'adfs_f_update' at fs/adfs/dir_f.c:403:2:
> > include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
> >     __read_overflow2();
> >     ^~~~~~~~~~~~~~~~~~
> >
> > The warning is correct in the sense that a negative 'pos' argument
> > to the function would have that result. However, this is not a bug,
> > as we know the position is always positive (in fact, between 5
> > and 2007, inclusive) when the function gets called.
> >
> > Changing the variable to a unsigned type avoids the problem. I decided
> > to use 'unsigned int' for the position in the directory and the block
> > number, as they are both counting things, but use size_t for the
> > offset and length that get passed into memcpy. This shuts up the
> > warning.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>  
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> Thanks for the fix! (Added sfr to Cc since he noticed this too.)

Can someone please send me the patch so I can use it if Andrew doesn't
get around to updating mmotd today?

-- 
Cheers,
Stephen Rothwell

[toc] | [prev] | [next] | [standalone]

#1701330

On Tue, Aug 1, 2017 at 5:04 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"),
> we get a warning about possible stack overflow from a memcpy that
> was not strictly bounded to the size of the local variable:
>
>     inlined from 'ext4_mb_seq_groups_show' at fs/ext4/mballoc.c:2322:2:
> include/linux/string.h:309:9: error: '__builtin_memcpy': writing between 161 and 1116 bytes into a region of size 160 overflows the destination [-Werror=stringop-overflow=]
>
> We actually had a bug here that would have been found by the warning,
> but it was already fixed last year in commit 30a9d7afe70e ("ext4: fix
> stack memory corruption with 64k block size").
>
> This replaces the fixed-length structure on the stack with a variable-length
> structure, using the correct upper bound that tells the compiler that
> everything is really fine here. I also change the loop count to check
> for the same upper bound for consistency, but the existing code is
> already correct here.
>
> Note that while clang won't allow certain kinds of variable-length arrays
> in structures, this particular instance is fine, as the array is at the
> end of the structure, and the size is strictly bounded.
>
> There is one remaining issue with the function that I'm not addressing
> here: With s_blocksize_bits==16, we don't actually print the last two
> members of the array, as we loop though just the first 14 members.
> This could be easily addressed by adding two extra columns in the output,
> but that could in theory break parsers in user space, and should be
> a separate patch if we decide to modify it.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

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

-Kees

> ---
>  fs/ext4/mballoc.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 581e357e8406..803cab1939fe 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2295,9 +2295,12 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
>         int err, buddy_loaded = 0;
>         struct ext4_buddy e4b;
>         struct ext4_group_info *grinfo;
> +       unsigned char blocksize_bits = min_t(unsigned char,
> +                                            sb->s_blocksize_bits,
> +                                            EXT4_MAX_BLOCK_LOG_SIZE);
>         struct sg {
>                 struct ext4_group_info info;
> -               ext4_grpblk_t counters[EXT4_MAX_BLOCK_LOG_SIZE + 2];
> +               ext4_grpblk_t counters[blocksize_bits + 2];
>         } sg;
>
>         group--;
> @@ -2306,8 +2309,6 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
>                               " 2^0   2^1   2^2   2^3   2^4   2^5   2^6  "
>                               " 2^7   2^8   2^9   2^10  2^11  2^12  2^13  ]\n");
>
> -       i = (sb->s_blocksize_bits + 2) * sizeof(sg.info.bb_counters[0]) +
> -               sizeof(struct ext4_group_info);
>         grinfo = ext4_get_group_info(sb, group);
>         /* Load the group info in memory only if not already loaded. */
>         if (unlikely(EXT4_MB_GRP_NEED_INIT(grinfo))) {
> @@ -2319,7 +2320,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
>                 buddy_loaded = 1;
>         }
>
> -       memcpy(&sg, ext4_get_group_info(sb, group), i);
> +       memcpy(&sg, ext4_get_group_info(sb, group), sizeof(sg));
>
>         if (buddy_loaded)
>                 ext4_mb_unload_buddy(&e4b);
> @@ -2327,7 +2328,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
>         seq_printf(seq, "#%-5u: %-5u %-5u %-5u [", group, sg.info.bb_free,
>                         sg.info.bb_fragments, sg.info.bb_first_free);
>         for (i = 0; i <= 13; i++)
> -               seq_printf(seq, " %-5u", i <= sb->s_blocksize_bits + 1 ?
> +               seq_printf(seq, " %-5u", i <= blocksize_bits + 1 ?
>                                 sg.info.bb_counters[i] : 0);
>         seq_printf(seq, " ]\n");
>
> --
> 2.9.0
>



-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1704726

On Tue, Aug 01, 2017 at 02:04:03PM +0200, Arnd Bergmann wrote:
> There is one remaining issue with the function that I'm not addressing
> here: With s_blocksize_bits==16, we don't actually print the last two
> members of the array, as we loop though just the first 14 members.
> This could be easily addressed by adding two extra columns in the output,
> but that could in theory break parsers in user space, and should be
> a separate patch if we decide to modify it.

Actually, the counters array is blocksize_bits+2 in length.  So for
all block sizes greater than 4k (blocksize_bits == 12), we're not
iterating over all of the free space counters maintained by mballoc.
However, since most Linux systems run architectures where the page
size is 4k, and the Linux VM really doesn't easily support file system
block sizes greater than the page size, this really isn't an issue
except on Itanics and Power systems.

I very much doubt there are userspace parsers who depend on this,
since far too many programmers subscribe to the "All the world's an
x86" theory, in direct contravention of Henry Spencer's Tenth
commandment:

	https://www.lysator.liu.se/c/ten-commandments.html

But indeed, it's a separate patch for another day.

Thanks, I'll apply this patch.

						- Ted

[toc] | [prev] | [next] | [standalone]

#1704967

On Sun, Aug 6, 2017 at 3:53 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Tue, Aug 01, 2017 at 02:04:03PM +0200, Arnd Bergmann wrote:
>> There is one remaining issue with the function that I'm not addressing
>> here: With s_blocksize_bits==16, we don't actually print the last two
>> members of the array, as we loop though just the first 14 members.
>> This could be easily addressed by adding two extra columns in the output,
>> but that could in theory break parsers in user space, and should be
>> a separate patch if we decide to modify it.
>
> Actually, the counters array is blocksize_bits+2 in length.  So for
> all block sizes greater than 4k (blocksize_bits == 12), we're not
> iterating over all of the free space counters maintained by mballoc.

Ah, makes sense.

> However, since most Linux systems run architectures where the page
> size is 4k, and the Linux VM really doesn't easily support file system
> block sizes greater than the page size, this really isn't an issue
> except on Itanics and Power systems.

Red Hat also build their arm64 kernels with 64k pages for some odd
reason I could never quite understand.

> Thanks, I'll apply this patch.

Thanks!

     Arnd

[toc] | [prev] | [next] | [standalone]

#1705126

On Tuesday, August 1, 2017 5:34:03 PM IST Arnd Bergmann wrote:
> After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"),
> we get a warning about possible stack overflow from a memcpy that
> was not strictly bounded to the size of the local variable:
> 
>     inlined from 'ext4_mb_seq_groups_show' at fs/ext4/mballoc.c:2322:2:
> include/linux/string.h:309:9: error: '__builtin_memcpy': writing between 161 and 1116 bytes into a region of size 160 overflows the destination [-Werror=stringop-overflow=]
> 
> We actually had a bug here that would have been found by the warning,
> but it was already fixed last year in commit 30a9d7afe70e ("ext4: fix
> stack memory corruption with 64k block size").
> 
> This replaces the fixed-length structure on the stack with a variable-length
> structure, using the correct upper bound that tells the compiler that
> everything is really fine here. I also change the loop count to check
> for the same upper bound for consistency, but the existing code is
> already correct here.
> 
> Note that while clang won't allow certain kinds of variable-length arrays
> in structures, this particular instance is fine, as the array is at the
> end of the structure, and the size is strictly bounded.
> 
> There is one remaining issue with the function that I'm not addressing
> here: With s_blocksize_bits==16, we don't actually print the last two
> members of the array, as we loop though just the first 14 members.
> This could be easily addressed by adding two extra columns in the output,
> but that could in theory break parsers in user space, and should be
> a separate patch if we decide to modify it.
> 

I executed xfstests on a ppc64 machine with both 4k and 64k block size 
combination.

Tested-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

-- 
chandan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web