kasan: detect negative size in memory operation function

Patch series "fix the missing underflow in memory operation function", v4. The patchset helps to produce a KASAN report when size is negative in memory operation functions. It is helpful for programmer to solve an undefined behavior issue. Patch 1 based on Dmitry's review and suggestion, patch 2 is a test in order to verify the patch 1. [1]https://bugzilla.kernel.org/show_bug.cgi?id=199341 [2]https://lore.kernel.org/linux-arm-kernel/20190927034338.15813-1-walter-zh.wu@mediatek.com/ This patch (of 2): KASAN missed detecting size is a negative number in memset(), memcpy(), and memmove(), it will cause out-of-bounds bug. So needs to be detected by KASAN. If size is a negative number, then it has a reason to be defined as out-of-bounds bug type. Casting negative numbers to size_t would indeed turn up as a large size_t and its value will be larger than ULONG_MAX/2, so that this can qualify as out-of-bounds. KASAN report is shown below: BUG: KASAN: out-of-bounds in kmalloc_memmove_invalid_size+0x70/0xa0 Read of size 18446744073709551608 at addr ffffff8069660904 by task cat/72 CPU: 2 PID: 72 Comm: cat Not tainted 5.4.0-rc1-next-20191004ajb-00001-gdb8af2f372b2-dirty #1 Hardware name: linux,dummy-virt (DT) Call trace: dump_backtrace+0x0/0x288 show_stack+0x14/0x20 dump_stack+0x10c/0x164 print_address_description.isra.9+0x68/0x378 __kasan_report+0x164/0x1a0 kasan_report+0xc/0x18 check_memory_region+0x174/0x1d0 memmove+0x34/0x88 kmalloc_memmove_invalid_size+0x70/0xa0 [1] https://bugzilla.kernel.org/show_bug.cgi?id=199341 [cai@lca.pw: fix -Wdeclaration-after-statement warn] Link: http://lkml.kernel.org/r/1583509030-27939-1-git-send-email-cai@lca.pw [peterz@infradead.org: fix objtool warning] Link: http://lkml.kernel.org/r/20200305095436.GV2596@hirez.programming.kicks-ass.net Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dmitry Vyukov <dvyukov@google.com> Suggested-by: Dmitry Vyukov <dvyukov@google.com> Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com> Signed-off-by: Qian Cai <cai@lca.pw> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Reviewed-by: Dmitry Vyukov <dvyukov@google.com> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com> Cc: Alexander Potapenko <glider@google.com> Link: http://lkml.kernel.org/r/20191112065302.7015-1-walter-zh.wu@mediatek.com Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Walter Wu committed 6 years ago

Commit 8cceeff1 parent 40271498 files · +54 −21Browse files

Unified · Split

8 files

include/linux/kasan.h +1 −1
mm/kasan/common.c +19 −7
mm/kasan/generic.c +5 −4
mm/kasan/generic_report.c +11 −0
mm/kasan/kasan.h +1 −1
mm/kasan/report.c +1 −4
mm/kasan/tags.c +5 −4
mm/kasan/tags_report.c +11 −0

include/linux/kasan.h+1−1

@@ -190,7 +190,7 @@ void kasan_init_tags(void); · 189 unmodified lines
190	190
191	191	void kasan_reset_tag(const void addr);
192	192
193		−void kasan_report(unsigned long addr, size_t size,
	193	+bool kasan_report(unsigned long addr, size_t size,
194	194	bool is_write, unsigned long ip);
195	195
196	196	#else /* CONFIG_KASAN_SW_TAGS */

mm/kasan/common.c+19−7

@@ -105,7 +105,8 @@ EXPORT_SYMBOL(__kasan_check_write); · 104 unmodified lines
105	105	#undef memset
106	106	void memset(void addr, int c, size_t len)
107	107	{
108		− check_memory_region((unsigned long)addr, len, true, _RET_IP_);
	108	+ if (!check_memory_region((unsigned long)addr, len, true, _RET_IP_))
	109	+ return NULL;
109	110
110	111	return __memset(addr, c, len);
111	112	}
@@ -114,8 +115,9 @@ void memset(void addr, int c, size_t len) · 2 unmodified lines
114	115	#undef memmove
115	116	void memmove(void dest, const void *src, size_t len)
116	117	{
117		− check_memory_region((unsigned long)src, len, false, _RET_IP_);
118		− check_memory_region((unsigned long)dest, len, true, _RET_IP_);
	118	+ if (!check_memory_region((unsigned long)src, len, false, _RET_IP_) \|\|
	119	+ !check_memory_region((unsigned long)dest, len, true, _RET_IP_))
	120	+ return NULL;
119	121
120	122	return __memmove(dest, src, len);
121	123	}
@@ -124,8 +126,9 @@ void memmove(void dest, const void *src, size_t len) · 2 unmodified lines
124	126	#undef memcpy
125	127	void memcpy(void dest, const void *src, size_t len)
126	128	{
127		− check_memory_region((unsigned long)src, len, false, _RET_IP_);
128		− check_memory_region((unsigned long)dest, len, true, _RET_IP_);
	129	+ if (!check_memory_region((unsigned long)src, len, false, _RET_IP_) \|\|
	130	+ !check_memory_region((unsigned long)dest, len, true, _RET_IP_))
	131	+ return NULL;
129	132
130	133	return __memcpy(dest, src, len);
131	134	}
@@ -634,12 +637,21 @@ void kasan_free_shadow(const struct vm_struct *vm) · 502 unmodified lines
634	637	#endif
635	638
636	639	extern void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip);
	640	+extern bool report_enabled(void);
637	641
638		−void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
	642	+bool kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
639	643	{
640	644	unsigned long flags = user_access_save();
641		− __kasan_report(addr, size, is_write, ip);
	645	+ bool ret = false;
	646	+
	647	+ if (likely(report_enabled())) {
	648	+ __kasan_report(addr, size, is_write, ip);
	649	+ ret = true;
	650	+ }
	651	+
642	652	user_access_restore(flags);
	653	+
	654	+ return ret;
643	655	}
644	656
645	657	#ifdef CONFIG_MEMORY_HOTPLUG

mm/kasan/generic.c+5−4

@@ -173,17 +173,18 @@ static __always_inline bool check_memory_region_inline(unsigned long addr, · 172 unmodified lines
173	173	if (unlikely(size == 0))
174	174	return true;
175	175
	176	+ if (unlikely(addr + size < addr))
	177	+ return !kasan_report(addr, size, write, ret_ip);
	178	+
176	179	if (unlikely((void *)addr <
177	180	kasan_shadow_to_mem((void *)KASAN_SHADOW_START))) {
178		− kasan_report(addr, size, write, ret_ip);
179		− return false;
	181	+ return !kasan_report(addr, size, write, ret_ip);
180	182	}
181	183
182	184	if (likely(!memory_is_poisoned(addr, size)))
183	185	return true;
184	186
185		− kasan_report(addr, size, write, ret_ip);
186		− return false;
	187	+ return !kasan_report(addr, size, write, ret_ip);
187	188	}
188	189
189	190	bool check_memory_region(unsigned long addr, size_t size, bool write,

mm/kasan/generic_report.c+11−0

@@ -110,6 +110,17 @@ static const char get_wild_bug_type(struct kasan_access_info info) · 109 unmodified lines
110	110
111	111	const char get_bug_type(struct kasan_access_info info)
112	112	{
	113	+ /*
	114	+ * If access_size is a negative number, then it has reason to be
	115	+ * defined as out-of-bounds bug type.
	116	+ *
	117	+ * Casting negative numbers to size_t would indeed turn up as
	118	+ * a large size_t and its value will be larger than ULONG_MAX/2,
	119	+ * so that this can qualify as out-of-bounds.
	120	+ */
	121	+ if (info->access_addr + info->access_size < info->access_addr)
	122	+ return "out-of-bounds";
	123	+
113	124	if (addr_has_shadow(info->access_addr))
114	125	return get_shadow_bug_type(info);
115	126	return get_wild_bug_type(info);

mm/kasan/kasan.h+1−1

@@ -153,7 +153,7 @@ bool check_memory_region(unsigned long addr, size_t size, bool write, · 152 unmodified lines
153	153	void find_first_bad_addr(void addr, size_t size);
154	154	const char get_bug_type(struct kasan_access_info info);
155	155
156		−void kasan_report(unsigned long addr, size_t size,
	156	+bool kasan_report(unsigned long addr, size_t size,
157	157	bool is_write, unsigned long ip);
158	158	void kasan_report_invalid_free(void *object, unsigned long ip);
159	159

mm/kasan/report.c+1−4

@@ -446,7 +446,7 @@ static void print_shadow_for_address(const void *addr) · 445 unmodified lines
446	446	}
447	447	}
448	448
449		−static bool report_enabled(void)
	449	+bool report_enabled(void)
450	450	{
451	451	if (current->kasan_depth)
452	452	return false;
@@ -478,9 +478,6 @@ void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned lon · 25 unmodified lines
478	478	void *untagged_addr;
479	479	unsigned long flags;
480	480
481		− if (likely(!report_enabled()))
482		− return;
483		−
484	481	disable_trace_on_warning();
485	482
486	483	tagged_addr = (void *)addr;

mm/kasan/tags.c+5−4

@@ -86,6 +86,9 @@ bool check_memory_region(unsigned long addr, size_t size, bool write, · 85 unmodified lines
86	86	if (unlikely(size == 0))
87	87	return true;
88	88
	89	+ if (unlikely(addr + size < addr))
	90	+ return !kasan_report(addr, size, write, ret_ip);
	91	+
89	92	tag = get_tag((const void *)addr);
90	93
91	94	/*
@@ -111,15 +114,13 @@ bool check_memory_region(unsigned long addr, size_t size, bool write, · 19 unmodified lines
111	114	untagged_addr = reset_tag((const void *)addr);
112	115	if (unlikely(untagged_addr <
113	116	kasan_shadow_to_mem((void *)KASAN_SHADOW_START))) {
114		− kasan_report(addr, size, write, ret_ip);
115		− return false;
	117	+ return !kasan_report(addr, size, write, ret_ip);
116	118	}
117	119	shadow_first = kasan_mem_to_shadow(untagged_addr);
118	120	shadow_last = kasan_mem_to_shadow(untagged_addr + size - 1);
119	121	for (shadow = shadow_first; shadow <= shadow_last; shadow++) {
120	122	if (*shadow != tag) {
121		− kasan_report(addr, size, write, ret_ip);
122		− return false;
	123	+ return !kasan_report(addr, size, write, ret_ip);
123	124	}
124	125	}
125	126

mm/kasan/tags_report.c+11−0

@@ -60,6 +60,17 @@ const char get_bug_type(struct kasan_access_info info) · 59 unmodified lines
60	60	}
61	61
62	62	#endif
	63	+ /*
	64	+ * If access_size is a negative number, then it has reason to be
	65	+ * defined as out-of-bounds bug type.
	66	+ *
	67	+ * Casting negative numbers to size_t would indeed turn up as
	68	+ * a large size_t and its value will be larger than ULONG_MAX/2,
	69	+ * so that this can qualify as out-of-bounds.
	70	+ */
	71	+ if (info->access_addr + info->access_size < info->access_addr)
	72	+ return "out-of-bounds";
	73	+
63	74	return "invalid-access";
64	75	}
65	76

kasan: detect negative size in memory operation function

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code