[PATCH] mm/kfence: fix a potential NULL pointer dereference

* [PATCH] mm/kfence: fix a potential NULL pointer dereference
@ 2022-04-27  7:11 cgel.zte
  2022-04-27  7:33 ` Marco Elver
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: cgel.zte @ 2022-04-27  7:11 UTC (permalink / raw)
  To: glider, elver, akpm
  Cc: dvyukov, kasan-dev, linux-kernel, linux-mm, xu xin, Zeal Robot

From: xu xin <xu.xin16@zte.com.cn>

In __kfence_free(), the returned 'meta' from addr_to_metadata()
might be NULL just as the implementation of addr_to_metadata()
shows.

Let's add a check of the pointer 'meta' to avoid NULL pointer
dereference. The patch brings three changes:

1. Add checks in both kfence_free() and __kfence_free();
2. kfence_free is not inline function any longer and new inline
   function '__try_free_kfence_meta' is introduced.
3. The check of is_kfence_address() is not required for
__kfence_free() now because __kfence_free has done the check in
addr_to_metadata();

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: xu xin <xu.xin16@zte.com.cn>
---
 include/linux/kfence.h | 10 ++--------
 mm/kfence/core.c       | 30 +++++++++++++++++++++++++++---
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/include/linux/kfence.h b/include/linux/kfence.h
index 726857a4b680..fbf6391ab53c 100644
--- a/include/linux/kfence.h
+++ b/include/linux/kfence.h
@@ -160,7 +160,7 @@ void *kfence_object_start(const void *addr);
  * __kfence_free() - release a KFENCE heap object to KFENCE pool
  * @addr: object to be freed
  *
- * Requires: is_kfence_address(addr)
+ * Requires: is_kfence_address(addr), but now it's unnecessary
  *
  * Release a KFENCE object and mark it as freed.
  */
@@ -179,13 +179,7 @@ void __kfence_free(void *addr);
  * allocator's free codepath. The allocator must check the return value to
  * determine if it was a KFENCE object or not.
  */
-static __always_inline __must_check bool kfence_free(void *addr)
-{
-	if (!is_kfence_address(addr))
-		return false;
-	__kfence_free(addr);
-	return true;
-}
+bool __must_check kfence_free(void *addr);
 
 /**
  * kfence_handle_page_fault() - perform page fault handling for KFENCE pages
diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 6e69986c3f0d..1405585369b3 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -1048,10 +1048,10 @@ void *kfence_object_start(const void *addr)
 	return meta ? (void *)meta->addr : NULL;
 }
 
-void __kfence_free(void *addr)
-{
-	struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr);
 
+/* Require: meta is not NULL*/
+static __always_inline void __try_free_kfence_meta(struct kfence_metadata *meta)
+{
 #ifdef CONFIG_MEMCG
 	KFENCE_WARN_ON(meta->objcg);
 #endif
@@ -1067,6 +1067,30 @@ void __kfence_free(void *addr)
 		kfence_guarded_free(addr, meta, false);
 }
 
+void __kfence_free(void *addr)
+{
+	struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr);
+
+	if (!meta) {
+		kfence_report_error(addr, false, NULL, NULL, KFENCE_ERROR_INVALID);
+		return;
+	}
+
+	__try_free_kfence_meta(meta);
+}
+
+bool __must_check kfence_free(void *addr)
+{
+	struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr);
+
+	if (!meta)
+		return false;
+
+	__try_free_kfence_meta(meta);
+
+	return true;
+}
+
 bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs *regs)
 {
 	const int page_index = (addr - (unsigned long)__kfence_pool) / PAGE_SIZE;
-- 
2.25.1


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