* [PATCH] mm/kfence: fix null pointer dereference on pointer meta @ 2021-10-23 17:18 Chengfeng Ye 2021-10-23 18:47 ` Marco Elver 0 siblings, 1 reply; 3+ messages in thread From: Chengfeng Ye @ 2021-10-23 17:18 UTC (permalink / raw) To: glider, elver, akpm Cc: dvyukov, kasan-dev, linux-mm, linux-kernel, Chengfeng Ye The pointer meta return from addr_to_metadata could be null, so there is a potential null pointer dereference issue. Fix this by adding a null check before dereference. Fixes: 0ce20dd8 ("mm: add Kernel Electric-Fence infrastructure") Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk> --- mm/kfence/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/kfence/core.c b/mm/kfence/core.c index 7a97db8bc8e7..7d2ec787e921 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -811,7 +811,7 @@ void __kfence_free(void *addr) * objects once it has been freed. meta->cache may be NULL if the cache * was destroyed. */ - if (unlikely(meta->cache && (meta->cache->flags & SLAB_TYPESAFE_BY_RCU))) + if (unlikely(meta && meta->cache && (meta->cache->flags & SLAB_TYPESAFE_BY_RCU))) call_rcu(&meta->rcu_head, rcu_guarded_free); else kfence_guarded_free(addr, meta, false); -- 2.17.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] mm/kfence: fix null pointer dereference on pointer meta 2021-10-23 17:18 [PATCH] mm/kfence: fix null pointer dereference on pointer meta Chengfeng Ye @ 2021-10-23 18:47 ` Marco Elver [not found] ` <TYCP286MB1188F7FAA423CFA03225B3BE8A819@TYCP286MB1188.JPNP286.PROD.OUTLOOK.COM> 0 siblings, 1 reply; 3+ messages in thread From: Marco Elver @ 2021-10-23 18:47 UTC (permalink / raw) To: Chengfeng Ye; +Cc: glider, akpm, dvyukov, kasan-dev, linux-mm, linux-kernel On Sat, 23 Oct 2021 at 19:20, Chengfeng Ye <cyeaa@connect.ust.hk> wrote: > The pointer meta return from addr_to_metadata could be null, so > there is a potential null pointer dereference issue. Fix this > by adding a null check before dereference. > > Fixes: 0ce20dd8 ("mm: add Kernel Electric-Fence infrastructure") > Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk> > --- > mm/kfence/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > index 7a97db8bc8e7..7d2ec787e921 100644 > --- a/mm/kfence/core.c > +++ b/mm/kfence/core.c > @@ -811,7 +811,7 @@ void __kfence_free(void *addr) > * objects once it has been freed. meta->cache may be NULL if the cache > * was destroyed. > */ > - if (unlikely(meta->cache && (meta->cache->flags & SLAB_TYPESAFE_BY_RCU))) > + if (unlikely(meta && meta->cache && (meta->cache->flags & SLAB_TYPESAFE_BY_RCU))) > call_rcu(&meta->rcu_head, rcu_guarded_free); > else > kfence_guarded_free(addr, meta, false); Sorry -- Nack. What bug did you encounter? Please see [1], and I'm afraid this attempt makes even less sense because if it were (hypothetically) NULL like you say we just call kfence_guarded_free() and crash there. [1] https://lkml.kernel.org/r/CANpmjNMcgUsdvXrvQHn+-y1w-z-6QAS+WJ27RB2DCnVxORRcuw@mail.gmail.com However, what I wrote in [1] equally applies here: > [...] > Adding a check like this could also hide genuine bugs, as meta should > never be NULL in __kfence_free(). If it is, we'd like to see a crash. > > Did you read kfence_free() in include/linux/kfence.h? It already > prevents __kfence_free() being called with a non-KFENCE address. > > Without a more thorough explanation, Nack. May I ask which static analysis tool keeps flagging this? Thanks, -- Marco ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <TYCP286MB1188F7FAA423CFA03225B3BE8A819@TYCP286MB1188.JPNP286.PROD.OUTLOOK.COM>]
* Re: [PATCH] mm/kfence: fix null pointer dereference on pointer meta [not found] ` <TYCP286MB1188F7FAA423CFA03225B3BE8A819@TYCP286MB1188.JPNP286.PROD.OUTLOOK.COM> @ 2021-10-25 6:00 ` Marco Elver 0 siblings, 0 replies; 3+ messages in thread From: Marco Elver @ 2021-10-25 6:00 UTC (permalink / raw) To: YE Chengfeng; +Cc: kasan-dev, LKML On Sat, 23 Oct 2021 at 21:22, YE Chengfeng <cyeaa@connect.ust.hk> wrote: [...] > Thanks for your reply, this is reported by a static analysis tool developed by us. It just checks dataflow and doesn't know other complex semantics. I didn't know whether it is a real bug, so I send the patch just in case. It seems that if the index is incorrect, the function addr_to_metadata will also return null-ptr, I don't know whether this is checked by other upper-level functions. [...] > And you are right, if it is a null-ptr, the root cause of it should be in the upper-level function. I think you can add some null-ptr check like assert(meta != null) if you want, this will suppress this kind of false positive report. Anyway, I think it is not a very good thing to just let this null-ptr dereference happen, even though it is not a big deal. Adding some checking to handle this case may be better, for example, print some error logging. It's a little more complicated than this: the negative index may happen when called with an object in range R = [__kfence_pool, __kfence_pool+(PAGE_SIZE*2)-1]. The first thing to note is that this address range is never returned by KFENCE as a valid object because both pages are "guard pages". Secondly, while calling kfence_free(R) will result in the NULL-deref, however, such a call is either buggy or malicious because it's only meant to be called from the allocators' kfree slow-path (slub.c and slab.c). Calling kfree(R) _does not_ lead to the kfree slow-path which calls kfence_free(), because the first 2 pages in KFENCE's pool do not have PageSlab nor page->slab_cache set. You can try it yourself by randomly doing a kfree(__kfence_pool) somewhere, and observing that nothing happens. As you can see, encountering the NULL-deref in __kfence_free() really should be impossible, unless something really bad is happening (e.g. malicious invocation, corrupt memory, bad CPU, etc.). And regarding assert(meta != null) you mentioned: the kernel does not have asserts, and the closest we have to asserts are WARN_ON() and BUG_ON(). That latter of which is closest to an assert() you may be familiar with from user space. However, its use is heavily discouraged: unlike user space, the kernel crashing takes the whole machine down. Therefore, the kernel wants to handle errors as gracefully as possible, i.e. recover where possible. However, something like BUG_ON(!ptr) is quite redundant, because a NULL-deref always crashes the kernel and also prints a helpful call trace. But as reasoned above, really shouldn't happen in our case. And if it does, we'd _really_ want to know about it (just crash) -- we either have a serious bug somewhere, or something more malicious is happening. Therefore, handling this case more gracefully, be it with a WARN_ON() or otherwise, does not seem appropriate as I couldn't say if it's safe to recover and continue execution in such a state. The same is true for any other place in the kernel handling pointers: if a NULL-deref really isn't expected, often it makes more sense to crash rather than continue in an unknown bad state potentially corrupting more data. Thanks, -- Marco ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-25 6:01 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-23 17:18 [PATCH] mm/kfence: fix null pointer dereference on pointer meta Chengfeng Ye 2021-10-23 18:47 ` Marco Elver [not found] ` <TYCP286MB1188F7FAA423CFA03225B3BE8A819@TYCP286MB1188.JPNP286.PROD.OUTLOOK.COM> 2021-10-25 6:00 ` Marco Elver
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.