| Message ID | 20170801120438.1582336-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 2df2c3402fc81918a888e1ec711369f6014471f2 |
| Headers | show |
| Series | [1/2] ext4: fix warning about stack corruption | expand |
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
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
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
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
Hi Arnd, > 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. Unfortunately it doesn't appear to work, at least with ppc64le clang: fs/ext4/mballoc.c:2303:17: error: fields must have a constant size: 'variable length array in structure' extension will never be supported ext4_grpblk_t counters[blocksize_bits + 2]; Anton
On Tue, Aug 22, 2017 at 1:08 PM, Anton Blanchard <anton@ozlabs.org> wrote: > Hi Arnd, >> >> 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. > > Unfortunately it doesn't appear to work, at least with ppc64le clang: > > fs/ext4/mballoc.c:2303:17: error: fields must have a constant size: 'variable length array in structure' extension will never be supported > ext4_grpblk_t counters[blocksize_bits + 2]; My fix for this is in the ext4/dev branch in linux-next, I hope it still makes it into v4.13. Arnd
> > Unfortunately it doesn't appear to work, at least with ppc64le > > clang: > > > > fs/ext4/mballoc.c:2303:17: error: fields must have a constant size: > > 'variable length array in structure' extension will never be > > supported ext4_grpblk_t counters[blocksize_bits + 2]; > > My fix for this is in the ext4/dev branch in linux-next, I hope it > still makes it into v4.13. Thanks Arnd, I see it now. Anton
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");
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(-) -- 2.9.0