All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] fortify: Use WARN instead of BUG for now
@ 2017-07-26 18:52 Kees Cook
  2017-07-26 19:55 ` Josh Poimboeuf
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Kees Cook @ 2017-07-26 18:52 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Linus Torvalds, Daniel Micay, Dan Williams, Mika Westerberg,
	Al Viro, David Howells, Heikki Krogerus, Bjorn Helgaas,
	Arnd Bergmann, Greg Kroah-Hartman, Mauro Carvalho Chehab,
	linux-kernel

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 <danielmicay@gmail.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] fortify: Use WARN instead of BUG for now
  2017-07-26 18:52 [PATCH v2] fortify: Use WARN instead of BUG for now Kees Cook
@ 2017-07-26 19:55 ` Josh Poimboeuf
  2017-07-26 21:33   ` Kees Cook
  2017-07-29  8:56 ` kbuild test robot
  2017-08-01 12:04 ` [PATCH 1/2] ext4: fix warning about stack corruption Arnd Bergmann
  2 siblings, 1 reply; 15+ messages in thread
From: Josh Poimboeuf @ 2017-07-26 19:55 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, Linus Torvalds, Daniel Micay, Dan Williams,
	Mika Westerberg, Al Viro, David Howells, Heikki Krogerus,
	Bjorn Helgaas, Arnd Bergmann, Greg Kroah-Hartman,
	Mauro Carvalho Chehab, linux-kernel

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] fortify: Use WARN instead of BUG for now
  2017-07-26 19:55 ` Josh Poimboeuf
@ 2017-07-26 21:33   ` Kees Cook
  0 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2017-07-26 21:33 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Andrew Morton, Linus Torvalds, Daniel Micay, Dan Williams,
	Mika Westerberg, Al Viro, David Howells, Heikki Krogerus,
	Bjorn Helgaas, Arnd Bergmann, Greg Kroah-Hartman,
	Mauro Carvalho Chehab, LKML

On Wed, Jul 26, 2017 at 12:55 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] fortify: Use WARN instead of BUG for now
  2017-07-26 18:52 [PATCH v2] fortify: Use WARN instead of BUG for now Kees Cook
  2017-07-26 19:55 ` Josh Poimboeuf
@ 2017-07-29  8:56 ` kbuild test robot
  2017-08-01 12:04 ` [PATCH 1/2] ext4: fix warning about stack corruption Arnd Bergmann
  2 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2017-07-29  8:56 UTC (permalink / raw)
  To: Kees Cook
  Cc: kbuild-all, Andrew Morton, Linus Torvalds, Daniel Micay,
	Dan Williams, Mika Westerberg, Al Viro, David Howells,
	Heikki Krogerus, Bjorn Helgaas, Arnd Bergmann,
	Greg Kroah-Hartman, Mauro Carvalho Chehab, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 8983 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] ext4: fix warning about stack corruption
  2017-07-26 18:52 [PATCH v2] fortify: Use WARN instead of BUG for now Kees Cook
  2017-07-26 19:55 ` Josh Poimboeuf
  2017-07-29  8:56 ` kbuild test robot
@ 2017-08-01 12:04 ` Arnd Bergmann
  2017-08-01 12:04   ` [PATCH 2/2] adfs: use 'unsigned' types for memcpy length Arnd Bergmann
                     ` (4 more replies)
  2 siblings, 5 replies; 15+ messages in thread
From: Arnd Bergmann @ 2017-08-01 12:04 UTC (permalink / raw)
  To: Theodore Ts'o, Andreas Dilger
  Cc: Kees Cook, Andrew Morton, Arnd Bergmann, Jan Kara,
	Chandan Rajendra, linux-ext4, linux-kernel

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] adfs: use 'unsigned' types for memcpy length
  2017-08-01 12:04 ` [PATCH 1/2] ext4: fix warning about stack corruption Arnd Bergmann
@ 2017-08-01 12:04   ` Arnd Bergmann
  2017-08-01 18:20     ` Kees Cook
  2017-08-01 18:26   ` [PATCH 1/2] ext4: fix warning about stack corruption Kees Cook
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Arnd Bergmann @ 2017-08-01 12:04 UTC (permalink / raw)
  Cc: Kees Cook, Andrew Morton, Arnd Bergmann, linux-kernel

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] adfs: use 'unsigned' types for memcpy length
  2017-08-01 12:04   ` [PATCH 2/2] adfs: use 'unsigned' types for memcpy length Arnd Bergmann
@ 2017-08-01 18:20     ` Kees Cook
  2017-08-01 21:43       ` Stephen Rothwell
  0 siblings, 1 reply; 15+ messages in thread
From: Kees Cook @ 2017-08-01 18:20 UTC (permalink / raw)
  To: Arnd Bergmann, Stephen Rothwell; +Cc: Andrew Morton, LKML

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] ext4: fix warning about stack corruption
  2017-08-01 12:04 ` [PATCH 1/2] ext4: fix warning about stack corruption Arnd Bergmann
  2017-08-01 12:04   ` [PATCH 2/2] adfs: use 'unsigned' types for memcpy length Arnd Bergmann
@ 2017-08-01 18:26   ` Kees Cook
  2017-08-06  1:53   ` Theodore Ts'o
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2017-08-01 18:26 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Theodore Ts'o, Andreas Dilger, Andrew Morton, Jan Kara,
	Chandan Rajendra, Ext4 Developers List, LKML

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] adfs: use 'unsigned' types for memcpy length
  2017-08-01 18:20     ` Kees Cook
@ 2017-08-01 21:43       ` Stephen Rothwell
  0 siblings, 0 replies; 15+ messages in thread
From: Stephen Rothwell @ 2017-08-01 21:43 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: Kees Cook, Andrew Morton, LKML

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] ext4: fix warning about stack corruption
  2017-08-01 12:04 ` [PATCH 1/2] ext4: fix warning about stack corruption Arnd Bergmann
  2017-08-01 12:04   ` [PATCH 2/2] adfs: use 'unsigned' types for memcpy length Arnd Bergmann
  2017-08-01 18:26   ` [PATCH 1/2] ext4: fix warning about stack corruption Kees Cook
@ 2017-08-06  1:53   ` Theodore Ts'o
  2017-08-06 20:34     ` Arnd Bergmann
  2017-08-07  6:42   ` Chandan Rajendra
  2017-08-22 11:08   ` Anton Blanchard
  4 siblings, 1 reply; 15+ messages in thread
From: Theodore Ts'o @ 2017-08-06  1:53 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andreas Dilger, Kees Cook, Andrew Morton, Jan Kara,
	Chandan Rajendra, linux-ext4, linux-kernel

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] ext4: fix warning about stack corruption
  2017-08-06  1:53   ` Theodore Ts'o
@ 2017-08-06 20:34     ` Arnd Bergmann
  0 siblings, 0 replies; 15+ messages in thread
From: Arnd Bergmann @ 2017-08-06 20:34 UTC (permalink / raw)
  To: Theodore Ts'o, Arnd Bergmann, Andreas Dilger, Kees Cook,
	Andrew Morton, Jan Kara, Chandan Rajendra, linux-ext4,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] ext4: fix warning about stack corruption
  2017-08-01 12:04 ` [PATCH 1/2] ext4: fix warning about stack corruption Arnd Bergmann
                     ` (2 preceding siblings ...)
  2017-08-06  1:53   ` Theodore Ts'o
@ 2017-08-07  6:42   ` Chandan Rajendra
  2017-08-22 11:08   ` Anton Blanchard
  4 siblings, 0 replies; 15+ messages in thread
From: Chandan Rajendra @ 2017-08-07  6:42 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Theodore Ts'o, Andreas Dilger, Kees Cook, Andrew Morton,
	Jan Kara, linux-ext4, linux-kernel, Theodore Ts'o

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] ext4: fix warning about stack corruption
  2017-08-01 12:04 ` [PATCH 1/2] ext4: fix warning about stack corruption Arnd Bergmann
                     ` (3 preceding siblings ...)
  2017-08-07  6:42   ` Chandan Rajendra
@ 2017-08-22 11:08   ` Anton Blanchard
  2017-08-22 11:57     ` Arnd Bergmann
  4 siblings, 1 reply; 15+ messages in thread
From: Anton Blanchard @ 2017-08-22 11:08 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Theodore Ts'o, Andreas Dilger, Kees Cook, Andrew Morton,
	Jan Kara, Chandan Rajendra, linux-ext4, linux-kernel

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] ext4: fix warning about stack corruption
  2017-08-22 11:08   ` Anton Blanchard
@ 2017-08-22 11:57     ` Arnd Bergmann
  2017-08-22 12:23       ` Anton Blanchard
  0 siblings, 1 reply; 15+ messages in thread
From: Arnd Bergmann @ 2017-08-22 11:57 UTC (permalink / raw)
  To: Anton Blanchard
  Cc: Theodore Ts'o, Andreas Dilger, Kees Cook, Andrew Morton,
	Jan Kara, Chandan Rajendra, linux-ext4,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] ext4: fix warning about stack corruption
  2017-08-22 11:57     ` Arnd Bergmann
@ 2017-08-22 12:23       ` Anton Blanchard
  0 siblings, 0 replies; 15+ messages in thread
From: Anton Blanchard @ 2017-08-22 12:23 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Theodore Ts'o, Andreas Dilger, Kees Cook, Andrew Morton,
	Jan Kara, Chandan Rajendra, linux-ext4,
	Linux Kernel Mailing List


> > 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-08-22 12:23 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-26 18:52 [PATCH v2] fortify: Use WARN instead of BUG for now Kees Cook
2017-07-26 19:55 ` Josh Poimboeuf
2017-07-26 21:33   ` Kees Cook
2017-07-29  8:56 ` kbuild test robot
2017-08-01 12:04 ` [PATCH 1/2] ext4: fix warning about stack corruption Arnd Bergmann
2017-08-01 12:04   ` [PATCH 2/2] adfs: use 'unsigned' types for memcpy length Arnd Bergmann
2017-08-01 18:20     ` Kees Cook
2017-08-01 21:43       ` Stephen Rothwell
2017-08-01 18:26   ` [PATCH 1/2] ext4: fix warning about stack corruption Kees Cook
2017-08-06  1:53   ` Theodore Ts'o
2017-08-06 20:34     ` Arnd Bergmann
2017-08-07  6:42   ` Chandan Rajendra
2017-08-22 11:08   ` Anton Blanchard
2017-08-22 11:57     ` Arnd Bergmann
2017-08-22 12:23       ` Anton Blanchard

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.