Enable run-time checking of dynamic memcpy() and memmove() lengths,
issuing a WARN when a write would exceed the size of the target struct
member, when built with CONFIG_FORTIFY_SOURCE=y. This would have
caught all of the memcpy()-based buffer overflows in the last 3 years,
specifically covering all the cases where the destination buffer size
is known at compile time.
This change ONLY adds a run-time warning. As false positives are currently
still expected, this will not block the overflow. The new warnings will
look like this:
memcpy: detected field-spanning write (size N) of single field "var->dest" (size M)
WARNING: CPU: n PID: pppp at source/file/path.c:nr function+0xXX/0xXX [module]
There may be false positives in the kernel where intentional
field-spanning writes are happening. These need to be addressed
similarly to how the compile-time cases were addressed: add a
struct_group(), split the memcpy(), or some other refactoring.
In order to make counting/investigating instances of added runtime checks
easier, each instance includes the destination variable name as a WARN
argument, prefixed with 'field "'. Therefore, on an x86_64 defconfig
build, it is trivial to inspect the build artifacts to find instances.
For example on an x86_64 defconfig build, there are 78 new run-time
memcpy() bounds checks added:
$ for i in vmlinux $(find . -name '*.ko'); do \
strings "$i" | grep '^field "'; done | wc -l
78
Simple cases where a destination buffer is known to be a dynamic size
do not generate a WARN. For example:
struct normal_flex_array {
void *a;
int b;
u32 c;
size_t array_size;
u8 flex_array[];
};
struct normal_flex_array *instance;
...
/* These will be ignored for run-time bounds checking. */
memcpy(instance, src, len);
memcpy(instance->flex_array, src, len);
However, one of the dynamic-sized destination cases is irritatingly
unable to be detected by the compiler: when using memcpy() to target
a composite struct member which contains a trailing flexible array
struct. For example:
struct wrapper {
int foo;
char bar;
struct normal_flex_array embedded;
};
struct wrapper *instance;
...
/* This will incorrectly WARN when len > sizeof(instance->embedded) */
memcpy(&instance->embedded, src, len);
These cases end up appearing to the compiler to be sized as if the
flexible array had 0 elements. :( For more details see:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
https://godbolt.org/z/vW6x8vh4P
These "composite flexible array structure destination" cases will be
need to be flushed out and addressed on a case-by-case basis.
Regardless, for the general case of using memcpy() on flexible array
destinations, future APIs will be created to handle common cases. Those
can be used to migrate away from open-coded memcpy() so that proper
error handling (instead of trapping) can be used.
As mentioned, none of these bounds checks block any overflows
currently. For users that have tested their workloads, do not encounter
any warnings, and wish to make these checks stop any overflows, they
can use a big hammer and set the sysctl panic_on_warn=1.
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/fortify-string.h | 70 ++++++++++++++++++++++++++++++++--
1 file changed, 67 insertions(+), 3 deletions(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 3b401fa0f374..ca3626c41785 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -3,6 +3,7 @@
#define _LINUX_FORTIFY_STRING_H_
#include <linux/const.h>
+#include <linux/bug.h>
#define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
#define __RENAME(x) __asm__(#x)
@@ -319,7 +320,7 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
* V = vulnerable to run-time overflow (will need refactoring to solve)
*
*/
-__FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,
+__FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
const size_t p_size,
const size_t q_size,
const size_t p_size_field,
@@ -368,16 +369,79 @@ __FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,
if ((p_size != (size_t)(-1) && p_size < size) ||
(q_size != (size_t)(-1) && q_size < size))
fortify_panic(func);
+
+ /*
+ * Warn when writing beyond destination field size.
+ *
+ * We must ignore p_size_field == 0 for existing 0-element
+ * fake flexible arrays, until they are all converted to
+ * proper flexible arrays.
+ *
+ * The implementation of __builtin_object_size() behaves
+ * like sizeof() when not directly referencing a flexible
+ * array member, which means there will be many bounds checks
+ * that will appear at run-time, without a way for them to be
+ * detected at compile-time (as can be done when the destination
+ * is specifically the flexible array member).
+ * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
+ */
+ if (p_size_field != 0 && p_size_field != (size_t)(-1) &&
+ p_size != p_size_field && p_size_field < size)
+ return true;
+
+ return false;
}
#define __fortify_memcpy_chk(p, q, size, p_size, q_size, \
p_size_field, q_size_field, op) ({ \
size_t __fortify_size = (size_t)(size); \
- fortify_memcpy_chk(__fortify_size, p_size, q_size, \
- p_size_field, q_size_field, #op); \
+ WARN_ONCE(fortify_memcpy_chk(__fortify_size, p_size, q_size, \
+ p_size_field, q_size_field, #op), \
+ #op ": detected field-spanning write (size %zu) of single %s (size %zu)\n", \
+ __fortify_size, \
+ "field \"" #p "\" at " __FILE__ ":" __stringify(__LINE__), \
+ p_size_field); \
__underlying_##op(p, q, __fortify_size); \
})
+/*
+ * Notes about compile-time buffer size detection:
+ *
+ * With these types...
+ *
+ * struct middle {
+ * u16 a;
+ * u8 middle_buf[16];
+ * int b;
+ * };
+ * struct end {
+ * u16 a;
+ * u8 end_buf[16];
+ * };
+ * struct flex {
+ * int a;
+ * u8 flex_buf[];
+ * };
+ *
+ * void func(TYPE *ptr) { ... }
+ *
+ * Cases where destination size cannot be currently detected:
+ * - the size of ptr's object (seemingly by design, gcc & clang fail):
+ * __builtin_object_size(ptr, 1) == -1
+ * - the size of flexible arrays in ptr's obj (by design, dynamic size):
+ * __builtin_object_size(ptr->flex_buf, 1) == -1
+ * - the size of ANY array at the end of ptr's obj (gcc and clang bug):
+ * __builtin_object_size(ptr->end_buf, 1) == -1
+ * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836
+ *
+ * Cases where destination size is currently detected:
+ * - the size of non-array members within ptr's object:
+ * __builtin_object_size(ptr->a, 1) == 2
+ * - the size of non-flexible-array in the middle of ptr's obj:
+ * __builtin_object_size(ptr->middle_buf, 1) == 16
+ *
+ */
+
/*
* __builtin_object_size() must be captured here to avoid evaluating argument
* side-effects further into the macro layers.
--
2.34.1
Hi Kees Cook,
the patch "[PATCH 1/2] fortify: Add run-time WARN for cross-field memcpy()"
raises a persistent WARNING as below report in our tests.
according to commit message, we understand this is kind of expected. but
we don't have enough knowledge if it reveals a real issue in kernel source
code and what the next step could be.
so we still report FYI.
if you think it's unnecessary for us to make out this kind of report, please
let us know. we will consider how to refine our report rules. Thanks a lot!
below is the full report.
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: 728833277da4a9643cb7ff62651ecb0124dfc610 ("[PATCH 1/2] fortify: Add run-time WARN for cross-field memcpy()")
url: https://github.com/intel-lab-lkp/linux/commits/Kees-Cook/fortify-Add-run-time-WARN-for-cross-field-memcpy/20220901-150051
base: https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git for-next/hardening
patch link: https://lore.kernel.org/lkml/[email protected]
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Configuring network interfaces... [ 20.451857][ T337] ------------[ cut here ]------------
[ 20.452641][ T337] memcpy: detected field-spanning write (size 40) of single field "&errmsg->msg" at net/netlink/af_netlink.c:2447 (size 16)
[ 20.454064][ T337] WARNING: CPU: 1 PID: 337 at net/netlink/af_netlink.c:2447 netlink_ack (net/netlink/af_netlink.c:2447 (discriminator 7))
[ 20.455040][ T337] Modules linked in: bochs drm_vram_helper drm_ttm_helper ttm sr_mod cdrom intel_rapl_msr drm_kms_helper intel_rapl_common ata_generic syscopyarea crct10dif_pclmul ppdev crc32_pclmul crc32c_intel sysfillrect sysimgblt fb_sys_fops ghash_clmulni_intel rapl ata_piix drm libata joydev parport_pc parport i2c_piix4 serio_raw
[ 20.458022][ T337] CPU: 1 PID: 337 Comm: ip Not tainted 6.0.0-rc2-00005-g728833277da4 #1
[ 20.458880][ T337] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-4 04/01/2014
[ 20.459868][ T337] RIP: 0010:netlink_ack (net/netlink/af_netlink.c:2447 (discriminator 7))
[ 20.460395][ T337] Code: 00 00 00 4c 89 ee 48 c7 c2 a0 14 e6 83 48 c7 c7 00 15 e6 83 4c 89 5c 24 20 4c 89 4c 24 18 c6 05 c9 b4 1a 02 01 e8 66 ea 58 00 <0f> 0b 4c 8b 5c 24 20 4c 8b 4c 24 18 e9 78 fc ff ff 83 c0 07 25 fc
All code
========
0: 00 00 add %al,(%rax)
2: 00 4c 89 ee add %cl,-0x12(%rcx,%rcx,4)
6: 48 c7 c2 a0 14 e6 83 mov $0xffffffff83e614a0,%rdx
d: 48 c7 c7 00 15 e6 83 mov $0xffffffff83e61500,%rdi
14: 4c 89 5c 24 20 mov %r11,0x20(%rsp)
19: 4c 89 4c 24 18 mov %r9,0x18(%rsp)
1e: c6 05 c9 b4 1a 02 01 movb $0x1,0x21ab4c9(%rip) # 0x21ab4ee
25: e8 66 ea 58 00 callq 0x58ea90
2a:* 0f 0b ud2 <-- trapping instruction
2c: 4c 8b 5c 24 20 mov 0x20(%rsp),%r11
31: 4c 8b 4c 24 18 mov 0x18(%rsp),%r9
36: e9 78 fc ff ff jmpq 0xfffffffffffffcb3
3b: 83 c0 07 add $0x7,%eax
3e: 25 .byte 0x25
3f: fc cld
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: 4c 8b 5c 24 20 mov 0x20(%rsp),%r11
7: 4c 8b 4c 24 18 mov 0x18(%rsp),%r9
c: e9 78 fc ff ff jmpq 0xfffffffffffffc89
11: 83 c0 07 add $0x7,%eax
14: 25 .byte 0x25
15: fc cld
[ 20.462364][ T337] RSP: 0018:ffffc90000f67770 EFLAGS: 00010286
[ 20.462989][ T337] RAX: 0000000000000000 RBX: ffffc90000f678a0 RCX: 0000000000000000
[ 20.463755][ T337] RDX: 0000000000000000 RSI: 1ffff920001eceb7 RDI: fffff520001ecee0
[ 20.464545][ T337] RBP: ffff88812300b800 R08: 0000000000000000 R09: ffffc90000f674a7
[ 20.465344][ T337] R10: fffff520001ece94 R11: 0000000000000001 R12: ffff88817bc47780
[ 20.466147][ T337] R13: 0000000000000028 R14: ffff888123009800 R15: ffff88817bc477b4
[ 20.466962][ T337] FS: 00007f5a7e1c4740(0000) GS:ffff88839d700000(0000) knlGS:0000000000000000
[ 20.467820][ T337] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 20.468483][ T337] CR2: 00007f5a7e2afb70 CR3: 0000000161e42000 CR4: 00000000000406e0
[ 20.469269][ T337] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 20.470132][ T337] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 20.470897][ T337] Call Trace:
[ 20.474440][ T337] <TASK>
[ 20.477798][ T337] ? rtnl_calcit+0x200/0x2c0
[ 20.481447][ T337] ? netlink_sendmsg (net/netlink/af_netlink.c:2405)
[ 20.485013][ T337] ? kasan_save_stack (mm/kasan/common.c:40)
[ 20.488546][ T337] ? kasan_save_stack (mm/kasan/common.c:39)
[ 20.492056][ T337] ? __kasan_slab_alloc (mm/kasan/common.c:45 mm/kasan/common.c:437 mm/kasan/common.c:470)
[ 20.499830][ T337] ? kmem_cache_alloc_lru (mm/slab.h:727 mm/slub.c:3243 mm/slub.c:3251 mm/slub.c:3258 mm/slub.c:3275)
[ 20.503341][ T337] ? proc_alloc_inode (fs/proc/inode.c:68)
[ 20.509279][ T337] ? alloc_inode (fs/inode.c:260)
[ 20.512708][ T337] netlink_rcv_skb (net/netlink/af_netlink.c:2507)
[ 20.516102][ T337] ? rtnl_calcit+0x2c0/0x2c0
[ 20.519486][ T337] ? netlink_ack (net/netlink/af_netlink.c:2478)
[ 20.522845][ T337] ? _copy_from_iter (lib/iov_iter.c:628 (discriminator 11))
[ 20.526084][ T337] netlink_unicast (net/netlink/af_netlink.c:1320 net/netlink/af_netlink.c:1345)
[ 20.529358][ T337] ? netlink_attachskb (net/netlink/af_netlink.c:1330)
[ 20.536737][ T337] ? check_heap_object (arch/x86/include/asm/bitops.h:207 arch/x86/include/asm/bitops.h:225 include/asm-generic/bitops/instrumented-non-atomic.h:142 include/linux/page-flags.h:487 mm/usercopy.c:193)
[ 20.540176][ T337] netlink_sendmsg (net/netlink/af_netlink.c:1921)
[ 20.543450][ T337] ? netlink_unicast (net/netlink/af_netlink.c:1841)
[ 20.546636][ T337] ? __import_iovec (lib/iov_iter.c:1771)
[ 20.549779][ T337] ? netlink_unicast (net/netlink/af_netlink.c:1841)
[ 20.552946][ T337] sock_sendmsg (net/socket.c:714 net/socket.c:734)
[ 20.556037][ T337] ____sys_sendmsg (net/socket.c:2482)
[ 20.559128][ T337] ? kernel_sendmsg (net/socket.c:2429)
[ 20.562184][ T337] ? __copy_msghdr (net/socket.c:2409)
[ 20.565212][ T337] ? __kasan_record_aux_stack (mm/kasan/generic.c:348)
[ 20.568287][ T337] ? call_rcu (arch/x86/include/asm/irqflags.h:29 (discriminator 3) arch/x86/include/asm/irqflags.h:70 (discriminator 3) arch/x86/include/asm/irqflags.h:106 (discriminator 3) kernel/rcu/tree.c:2794 (discriminator 3))
[ 20.571262][ T337] ? task_work_run (kernel/task_work.c:179 (discriminator 1))
[ 20.574198][ T337] ? exit_to_user_mode_loop (include/linux/resume_user_mode.h:49 kernel/entry/common.c:169)
[ 20.577141][ T337] ? exit_to_user_mode_prepare (kernel/entry/common.c:201)
[ 20.580055][ T337] ? syscall_exit_to_user_mode (arch/x86/include/asm/jump_label.h:27 include/linux/context_tracking_state.h:106 include/linux/context_tracking.h:41 kernel/entry/common.c:132 kernel/entry/common.c:296)
[ 20.582845][ T337] ___sys_sendmsg (net/socket.c:2538)
[ 20.585414][ T337] ? rcu_nocb_try_bypass (kernel/rcu/tree_nocb.h:382)
[ 20.587947][ T337] ? __ia32_sys_recvmmsg (net/socket.c:2525)
[ 20.590446][ T337] ? rcu_gp_kthread (kernel/rcu/tree_nocb.h:371)
[ 20.592791][ T337] ? fsnotify_grab_connector (fs/notify/mark.c:601)
[ 20.599331][ T337] ? rcu_segcblist_enqueue (arch/x86/include/asm/atomic64_64.h:46 include/linux/atomic/atomic-long.h:53 include/linux/atomic/atomic-instrumented.h:1295 kernel/rcu/rcu_segcblist.c:214 kernel/rcu/rcu_segcblist.c:231 kernel/rcu/rcu_segcblist.c:343)
[ 20.601685][ T337] ? call_rcu (arch/x86/include/asm/atomic64_64.h:22 include/linux/atomic/atomic-long.h:29 include/linux/atomic/atomic-instrumented.h:1266 kernel/rcu/rcu_segcblist.h:50 kernel/rcu/tree.c:2658 kernel/rcu/tree.c:2827)
[ 20.603842][ T337] ? _raw_write_lock_irq (kernel/locking/spinlock.c:153)
[ 20.606010][ T337] ? rcu_gp_kthread (kernel/rcu/tree_nocb.h:371)
[ 20.608185][ T337] ? kmem_cache_free (mm/slub.c:1780 mm/slub.c:3534 mm/slub.c:3551)
[ 20.610331][ T337] ? __fget_light (arch/x86/include/asm/atomic.h:29 include/linux/atomic/atomic-instrumented.h:28 fs/file.c:1005)
[ 20.612390][ T337] ? rcu_segcblist_enqueue (arch/x86/include/asm/atomic64_64.h:46 include/linux/atomic/atomic-long.h:53 include/linux/atomic/atomic-instrumented.h:1295 kernel/rcu/rcu_segcblist.c:214 kernel/rcu/rcu_segcblist.c:231 kernel/rcu/rcu_segcblist.c:343)
[ 20.614546][ T337] __sys_sendmsg (include/linux/file.h:31 net/socket.c:2567)
[ 20.616587][ T337] ? __sys_sendmsg_sock (net/socket.c:2553)
[ 20.618675][ T337] ? __fput (include/linux/percpu_counter.h:189 fs/file_table.c:58 fs/file_table.c:338)
[ 20.620624][ T337] ? exit_to_user_mode_loop (include/linux/sched.h:2305 include/linux/resume_user_mode.h:61 kernel/entry/common.c:169)
[ 20.622677][ T337] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
[ 20.624621][ T337] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
[ 20.626731][ T337] RIP: 0033:0x7f5a7e2beff4
[ 20.628638][ T337] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b5 0f 1f 80 00 00 00 00 48 8d 05 49 12 0c 00 8b 00 85 c0 75 13 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 41 89 d4 55 48 89 f5 53
All code
========
0: 00 f7 add %dh,%bh
2: d8 64 89 02 fsubs 0x2(%rcx,%rcx,4)
6: 48 c7 c0 ff ff ff ff mov $0xffffffffffffffff,%rax
d: eb b5 jmp 0xffffffffffffffc4
f: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
16: 48 8d 05 49 12 0c 00 lea 0xc1249(%rip),%rax # 0xc1266
1d: 8b 00 mov (%rax),%eax
1f: 85 c0 test %eax,%eax
21: 75 13 jne 0x36
23: b8 2e 00 00 00 mov $0x2e,%eax
28: 0f 05 syscall
2a:* 48 3d 00 f0 ff ff cmp $0xfffffffffffff000,%rax <-- trapping instruction
30: 77 54 ja 0x86
32: c3 retq
33: 0f 1f 00 nopl (%rax)
36: 41 54 push %r12
38: 41 89 d4 mov %edx,%r12d
3b: 55 push %rbp
3c: 48 89 f5 mov %rsi,%rbp
3f: 53 push %rbx
Code starting with the faulting instruction
===========================================
0: 48 3d 00 f0 ff ff cmp $0xfffffffffffff000,%rax
6: 77 54 ja 0x5c
8: c3 retq
9: 0f 1f 00 nopl (%rax)
c: 41 54 push %r12
e: 41 89 d4 mov %edx,%r12d
11: 55 push %rbp
12: 48 89 f5 mov %rsi,%rbp
15: 53 push %rbx
To reproduce:
# build kernel
cd linux
cp config-6.0.0-rc2-00005-g728833277da4 .config
make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Gustavo,
On Wed, Sep 07, 2022 at 08:39:19AM +0100, Gustavo A. R. Silva wrote:
> On Wed, Sep 07, 2022 at 01:42:16PM +0800, kernel test robot wrote:
>
> Hi!
>
> >
> > Hi Kees Cook,
> >
> > the patch "[PATCH 1/2] fortify: Add run-time WARN for cross-field memcpy()"
> > raises a persistent WARNING as below report in our tests.
> >
> > according to commit message, we understand this is kind of expected. but
> > we don't have enough knowledge if it reveals a real issue in kernel source
> > code and what the next step could be.
> >
> > so we still report FYI.
> >
> > if you think it's unnecessary for us to make out this kind of report, please
> > let us know. we will consider how to refine our report rules. Thanks a lot!
> >
> > below is the full report.
>
> It seems that the idea is to continue reporting these warnings, as they
> help us identify the places that need to be audited and determine how to
> refactor the code (in case it's a false positive), or how to properly fix
> it (in case it's an actual bug).
thanks a lot! very glad our report is helpful :)
>
> In this case, it seems that the issue was already addressed by this patch:
>
> https://lore.kernel.org/linux-hardening/[email protected]/
>
> Thanks
> --
> Gustavo
On Wed, Sep 07, 2022 at 01:42:16PM +0800, kernel test robot wrote:
Hi!
>
> Hi Kees Cook,
>
> the patch "[PATCH 1/2] fortify: Add run-time WARN for cross-field memcpy()"
> raises a persistent WARNING as below report in our tests.
>
> according to commit message, we understand this is kind of expected. but
> we don't have enough knowledge if it reveals a real issue in kernel source
> code and what the next step could be.
>
> so we still report FYI.
>
> if you think it's unnecessary for us to make out this kind of report, please
> let us know. we will consider how to refine our report rules. Thanks a lot!
>
> below is the full report.
It seems that the idea is to continue reporting these warnings, as they
help us identify the places that need to be audited and determine how to
refactor the code (in case it's a false positive), or how to properly fix
it (in case it's an actual bug).
In this case, it seems that the issue was already addressed by this patch:
https://lore.kernel.org/linux-hardening/[email protected]/
Thanks
--
Gustavo