Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <626902ab.1c69fb81.44f01.9c06@mx.google.com>
Date:   Wed, 27 Apr 2022 08:45:29 +0000
From:   CGEL <cgel.zte@gmail.com>
To:     Marco Elver <elver@google.com>
Cc:     glider@google.com, akpm@linux-foundation.org, dvyukov@google.com,
        kasan-dev@googlegroups.com, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, xu xin <xu.xin16@zte.com.cn>,
        Zeal Robot <zealci@zte.com.cn>
Subject: Re: [PATCH] mm/kfence: fix a potential NULL pointer dereference
References: <20220427071100.3844081-1-xu.xin16@zte.com.cn>
 <CANpmjNM8hKG+HH+pBR4cDLcU-sUWFO6t4CF89bt5uess0Zm3dg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANpmjNM8hKG+HH+pBR4cDLcU-sUWFO6t4CF89bt5uess0Zm3dg@mail.gmail.com>
Precedence: bulk

On Wed, Apr 27, 2022 at 09:33:52AM +0200, Marco Elver wrote:
> On Wed, 27 Apr 2022 at 09:11, <cgel.zte@gmail.com> wrote:
> >
> > 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.
> 
> 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@zte.com.cn>
> 
> 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@mail.gmail.com/
> 
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@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
> 
> (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.