While CONFIG_FORTIFY_SOURCE continues to shake out, don't unconditionally
use BUG(), opting instead for WARN(). This also renames fortify_panic()
to fortify_overflow() which better matches what it is being called for.
Cc: Daniel Micay <[email protected]>
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
v2:
- just do simple renaming, no logic changes (danielmicay)
Sending to akpm, since fortify went through -mm originally.
---
arch/x86/boot/compressed/misc.c | 2 +-
include/linux/string.h | 30 +++++++++++++++---------------
lib/string.c | 7 +++----
tools/objtool/check.c | 2 +-
4 files changed, 20 insertions(+), 21 deletions(-)
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index a0838ab929f2..c20cdc7cbd61 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -412,7 +412,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
return output;
}
-void fortify_panic(const char *name)
+void fortify_overflow(const char *name)
{
error("detected buffer overflow");
}
diff --git a/include/linux/string.h b/include/linux/string.h
index a467e617eeb0..25f47e07c4c6 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -197,7 +197,7 @@ static inline const char *kbasename(const char *path)
#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
#define __RENAME(x) __asm__(#x)
-void fortify_panic(const char *name) __noreturn __cold;
+void fortify_overflow(const char *name) __cold;
void __read_overflow(void) __compiletime_error("detected read beyond size of object passed as 1st parameter");
void __read_overflow2(void) __compiletime_error("detected read beyond size of object passed as 2nd parameter");
void __write_overflow(void) __compiletime_error("detected write beyond size of object passed as 1st parameter");
@@ -209,7 +209,7 @@ __FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
if (__builtin_constant_p(size) && p_size < size)
__write_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return __builtin_strncpy(p, q, size);
}
@@ -219,7 +219,7 @@ __FORTIFY_INLINE char *strcat(char *p, const char *q)
if (p_size == (size_t)-1)
return __builtin_strcat(p, q);
if (strlcat(p, q, p_size) >= p_size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return p;
}
@@ -231,7 +231,7 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
return __builtin_strlen(p);
ret = strnlen(p, p_size);
if (p_size <= ret)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return ret;
}
@@ -241,7 +241,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
size_t p_size = __builtin_object_size(p, 0);
__kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
if (p_size <= ret && maxlen != ret)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return ret;
}
@@ -260,7 +260,7 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
if (__builtin_constant_p(len) && len >= p_size)
__write_overflow();
if (len >= p_size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
__builtin_memcpy(p, q, len);
p[len] = '\0';
}
@@ -278,7 +278,7 @@ __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
p_len = strlen(p);
copy_len = strnlen(q, count);
if (p_size < p_len + copy_len + 1)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
__builtin_memcpy(p + p_len, q, copy_len);
p[p_len + copy_len] = '\0';
return p;
@@ -290,7 +290,7 @@ __FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size)
if (__builtin_constant_p(size) && p_size < size)
__write_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return __builtin_memset(p, c, size);
}
@@ -305,7 +305,7 @@ __FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size)
__read_overflow2();
}
if (p_size < size || q_size < size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return __builtin_memcpy(p, q, size);
}
@@ -320,7 +320,7 @@ __FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size)
__read_overflow2();
}
if (p_size < size || q_size < size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return __builtin_memmove(p, q, size);
}
@@ -331,7 +331,7 @@ __FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size)
if (__builtin_constant_p(size) && p_size < size)
__read_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return __real_memscan(p, c, size);
}
@@ -346,7 +346,7 @@ __FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size)
__read_overflow2();
}
if (p_size < size || q_size < size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return __builtin_memcmp(p, q, size);
}
@@ -356,7 +356,7 @@ __FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size)
if (__builtin_constant_p(size) && p_size < size)
__read_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return __builtin_memchr(p, c, size);
}
@@ -367,7 +367,7 @@ __FORTIFY_INLINE void *memchr_inv(const void *p, int c, size_t size)
if (__builtin_constant_p(size) && p_size < size)
__read_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return __real_memchr_inv(p, c, size);
}
@@ -378,7 +378,7 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)
if (__builtin_constant_p(size) && p_size < size)
__read_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_overflow(__func__);
return __real_kmemdup(p, size, gfp);
}
diff --git a/lib/string.c b/lib/string.c
index ebbb99c775bd..e8fc0c495442 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -979,9 +979,8 @@ char *strreplace(char *s, char old, char new)
}
EXPORT_SYMBOL(strreplace);
-void fortify_panic(const char *name)
+void fortify_overflow(const char *name)
{
- pr_emerg("detected buffer overflow in %s\n", name);
- BUG();
+ WARN(1, "detected buffer overflow in %s\n", name);
}
-EXPORT_SYMBOL(fortify_panic);
+EXPORT_SYMBOL(fortify_overflow);
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 2c6d74880403..9e45de4d2e72 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -156,7 +156,7 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
"kvm_spurious_fault",
"__reiserfs_panic",
"lbug_with_loc",
- "fortify_panic",
+ "fortify_overflow",
};
if (func->bind == STB_WEAK)
--
2.7.4
--
Kees Cook
Pixel Security
On Wed, Jul 26, 2017 at 11:52:19AM -0700, Kees Cook wrote:
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -156,7 +156,7 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
> "kvm_spurious_fault",
> "__reiserfs_panic",
> "lbug_with_loc",
> - "fortify_panic",
> + "fortify_overflow",
> };
If the function is no longer '__noreturn' then this entry needs to be
removed from the global_noreturns list.
--
Josh
On Wed, Jul 26, 2017 at 12:55 PM, Josh Poimboeuf <[email protected]> wrote:
> On Wed, Jul 26, 2017 at 11:52:19AM -0700, Kees Cook wrote:
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -156,7 +156,7 @@ static int __dead_end_function(struct objtool_file *file, struct symbol *func,
>> "kvm_spurious_fault",
>> "__reiserfs_panic",
>> "lbug_with_loc",
>> - "fortify_panic",
>> + "fortify_overflow",
>> };
>
> If the function is no longer '__noreturn' then this entry needs to be
> removed from the global_noreturns list.
Ah, dur, yes. Thanks! I see akpm has already merged this correction for -mm.
Thanks!
-Kees
--
Kees Cook
Pixel Security
Hi Kees,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.13-rc2 next-20170728]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kees-Cook/fortify-Use-WARN-instead-of-BUG-for-now/20170728-091210
config: ia64-sim_defconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64
All errors (new ones prefixed by >>):
lib/string.o: In function `fortify_overflow':
>> lib/string.c:984: undefined reference to `warn_slowpath_fmt'
vim +984 lib/string.c
981
982 void fortify_overflow(const char *name)
983 {
> 984 WARN(1, "detected buffer overflow in %s\n", name);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Tue, Aug 1, 2017 at 5:04 AM, Arnd Bergmann <[email protected]> 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 <[email protected]>
Acked-by: Kees Cook <[email protected]>
Thanks for the fix! (Added sfr to Cc since he noticed this too.)
-Kees
--
Kees Cook
Pixel Security
On Tue, Aug 1, 2017 at 5:04 AM, Arnd Bergmann <[email protected]> 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 <[email protected]>
Acked-by: Kees Cook <[email protected]>
-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
Hi Arnd,
On Tue, 1 Aug 2017 11:20:26 -0700 Kees Cook <[email protected]> wrote:
>
> On Tue, Aug 1, 2017 at 5:04 AM, Arnd Bergmann <[email protected]> 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 <[email protected]>
>
> Acked-by: Kees Cook <[email protected]>
>
> 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
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 <[email protected]> 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 <[email protected]>
--
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 <[email protected]> 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