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

[CGEL]

On Wed, Apr 27, 2022 at 09:33:52AM +0200, Marco Elver wrote:
> On Wed, 27 Apr 2022 at 09:11, <cgel.zte@xxxxxxxxx> wrote:
> >
> > From: xu xin <xu.xin16@xxxxxxxxxx>
> >
> > 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.
> 
> This is very bad for performance (see below).
> 
> > 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@xxxxxxxxxx>
> 
> Is this a static analysis robot? Please show a real stack trace with
> an actual NULL-deref.
> 
> Nack - please see:
> https://lore.kernel.org/all/CANpmjNO5-o1B9r2eYS_482RBVJSyPoHSnV2t+M8fJdFzBf6d2A@xxxxxxxxxxxxxx/
> 
Thanks for your reply. It's from static analysis indeed and no actual
NULL-deref event happened yet.

I'm just worried that what if address at the edge of __kfence_pool and
thus addr_to_metadata() returns NULL. Is is just a guess, I'm not sure.

But if __kfence_free make sure that the given address never is at the
edge of __kfence_pool, then the calculation and check in
addr_to_metadata()  is extra performance consumption:

	"index = (addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2) - 1;
	 if (index < 0 || index >= CONFIG_KFENCE_NUM_OBJECTS)  240
	 	return NULL;"


> >Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx>
> > ---
> >  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
> 
> (As an aside, something can't be required and be unnecessary at the same time.)

Oh, I'm sorry for this. In my opinion, inner addr_to_metadata(),
is_kfence_address is executed for the second time, so not necessary here.