Messages in this thread | | | From | Marco Elver <> | Date | Mon, 25 Oct 2021 08:00:00 +0200 | Subject | Re: [PATCH] mm/kfence: fix null pointer dereference on pointer meta |
| |
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
| |