Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211023171802.4693-1-cyeaa@connect.ust.hk> <CANpmjNP8uAexEZ3Qa-GfBfX6V8tAd7NK0vt3T3Xjh4CkzxfS-g@mail.gmail.com>
 <TYCP286MB1188F7FAA423CFA03225B3BE8A819@TYCP286MB1188.JPNP286.PROD.OUTLOOK.COM>
In-Reply-To: <TYCP286MB1188F7FAA423CFA03225B3BE8A819@TYCP286MB1188.JPNP286.PROD.OUTLOOK.COM>
From:   Marco Elver <elver@google.com>
Date:   Mon, 25 Oct 2021 08:00:00 +0200
Message-ID: <CANpmjNO5-o1B9r2eYS_482RBVJSyPoHSnV2t+M8fJdFzBf6d2A@mail.gmail.com>
Subject: Re: [PATCH] mm/kfence: fix null pointer dereference on pointer meta
To:     YE Chengfeng <cyeaa@connect.ust.hk>
Cc:     kasan-dev <kasan-dev@googlegroups.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 develop=
ed 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 ass=
ert(meta !=3D null) if you want, this will suppress this kind of false posi=
tive report. Anyway, I think it is not a very good thing to just let this n=
ull-ptr dereference happen, even though it is not a big deal. Adding some c=
hecking to handle this case may be better, for example, print some error lo=
gging.

It's a little more complicated than this: the negative index may
happen when called with an object in range R =3D [__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 !=3D 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