From: Kees Cook Subject: Re: [PATCH 1/2] ext4: fix warning about stack corruption Date: Tue, 1 Aug 2017 11:26:47 -0700 Message-ID: References: <20170726185219.GA57833@beast> <20170801120438.1582336-1-arnd@arndb.de> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: "Theodore Ts'o" , Andreas Dilger , Andrew Morton , Jan Kara , Chandan Rajendra , Ext4 Developers List , LKML To: Arnd Bergmann Return-path: Received: from mail-it0-f46.google.com ([209.85.214.46]:37595 "EHLO mail-it0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751928AbdHAS0t (ORCPT ); Tue, 1 Aug 2017 14:26:49 -0400 Received: by mail-it0-f46.google.com with SMTP id v127so12757656itd.0 for ; Tue, 01 Aug 2017 11:26:49 -0700 (PDT) In-Reply-To: <20170801120438.1582336-1-arnd@arndb.de> Sender: linux-ext4-owner@vger.kernel.org List-ID: On Tue, Aug 1, 2017 at 5:04 AM, 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. > > Signed-off-by: Arnd Bergmann Acked-by: Kees Cook -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