kfence: fix is_kfence_address() for addresses below KFENCE_POOL_SIZE

Originally the addr != NULL check was meant to take care of the case where __kfence_pool == NULL (KFENCE is disabled). However, this does not work for addresses where addr > 0 && addr < KFENCE_POOL_SIZE.

This can be the case on NULL-deref where addr > 0 && addr < PAGE_SIZE or any other faulting access with addr < KFENCE_POOL_SIZE. While the kernel would likely crash, the stack traces and report might be confusing due to double faults upon KFENCE's attempt to unprotect such an address.

Fix it by just checking that __kfence_pool != NULL instead.

Fixes 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure")

Reported-by Kuan-Ying Lee <[email protected]>

Signed-off-by Marco Elver <[email protected]>

Cc <[email protected]> [5.12+]

---
 include/linux/kfence.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

include/linux/kfence.h +0 -0

index a70d1ea03532..3fe6dd8a18c1 100644
@@ -51,10 +51,11 @@ extern atomic_t kfence_allocation_gate;
 static __always_inline bool is_kfence_address(const void *addr)
 {

+Cc Jann

Quoted 15 lines · Marco Elver

Originally the addr != NULL check was meant to take care of the case where __kfence_pool == NULL (KFENCE is disabled). However, this does not work for addresses where addr > 0 && addr < KFENCE_POOL_SIZE.

This can be the case on NULL-deref where addr > 0 && addr < PAGE_SIZE or any other faulting access with addr < KFENCE_POOL_SIZE. While the kernel would likely crash, the stack traces and report might be confusing due to double faults upon KFENCE's attempt to unprotect such an address.

Fix it by just checking that __kfence_pool != NULL instead.

Fixes 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure")

Reported-by Kuan-Ying Lee <[email protected]>

Signed-off-by Marco Elver <[email protected]>

Quoted 12 lines

Cc <[email protected]> [5.12+]

---
 include/linux/kfence.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

include/linux/kfence.h +0 -0

index a70d1ea03532..3fe6dd8a18c1 100644
@@ -51,10 +51,11 @@ extern atomic_t kfence_allocation_gate;
 static __always_inline bool is_kfence_address(const void *addr)
 {

include/linux/kfence.h extern atomic_t kfence_allocation_gate; · line 59

Quoted 10 lines

include/linux/kfence.h +4 -3

        /*
-        * The non-NULL check is required in case the __kfence_pool pointer was
-        * never initialized; keep it in the slow-path after the range-check.
+        * The __kfence_pool != NULL check is required to deal with the case
+        * where __kfence_pool == NULL && addr < KFENCE_POOL_SIZE. Keep it in
+        * the slow-path after the range-check!
         */
-       return unlikely((unsigned long)((char *)addr - __kfence_pool) < KFENCE_POOL_SIZE && addr);
+       return unlikely((unsigned long)((char *)addr - __kfence_pool) < KFENCE_POOL_SIZE && __kfence_pool);
 }

Jann, I recall discussing this check somewhere around:
https://lore.kernel.org/linux-doc/CAG48ez0D1+hStZaDOigwbqNqFHJAJtXK+8Nadeuiu1Byv+xp5A@mail.gmail.com/

I think you pointed out initially that we need another check, but somehow that turned into '&& addr' -- I think that's what we ended up with because of worry about another memory load, which is clearly wrong as that only works if addr==NULL. Simply checking __kfence_pool!=NULL is enough. I also checked codegen, and the compiler is smart enough to not reload the global __kfence_pool.

Wanted to call it out, just in case you see something even more efficient (probably the only way to do better is to get rid of the 2nd branch, which I don't think is possible). :-)

Thanks,
-- Marco