Re: [PATCH v5 1/2] kasan: catch invalid free before SLUB reinitializes the object

[Jann Horn <jannh@google.com>]

(I'll address the other feedback later)

On Thu, Aug 1, 2024 at 2:23 AM Andrey Konovalov <andreyknvl@gmail.com> wrote:
>
> On Tue, Jul 30, 2024 at 1:06 PM Jann Horn <jannh@google.com> wrote:
> >
> > Currently, when KASAN is combined with init-on-free behavior, the
> > initialization happens before KASAN's "invalid free" checks.
> >
> > More importantly, a subsequent commit will want to RCU-delay the actual
> > SLUB freeing of an object, and we'd like KASAN to still validate
> > synchronously that freeing the object is permitted. (Otherwise this
> > change will make the existing testcase kmem_cache_invalid_free fail.)
> >
> > So add a new KASAN hook that allows KASAN to pre-validate a
> > kmem_cache_free() operation before SLUB actually starts modifying the
> > object or its metadata.
[...]
> > @@ -503,15 +509,22 @@ bool __kasan_mempool_poison_object(void *ptr, unsigned long ip)
> >                 kasan_poison(ptr, folio_size(folio), KASAN_PAGE_FREE, false);
> >                 return true;
> >         }
> >
> >         if (is_kfence_address(ptr))
> >                 return false;
> > +       if (!kasan_arch_is_ready())
> > +               return true;
>
> Hm, I think we had a bug here: the function should return true in both
> cases. This seems reasonable: if KASAN is not checking the object, the
> caller can do whatever they want with it.

But if the object is a kfence allocation, we maybe do want the caller
to free it quickly so that kfence can catch potential UAF access? So
"return false" in that case seems appropriate. Or maybe we don't
because that makes the probability of catching an OOB access much
lower if the mempool is going to always return non-kfence allocations
as long as the pool isn't empty? Also I guess whether kfence vetoes
reuse of kfence objects probably shouldn't depend on whether the
kernel is built with KASAN... so I guess it would be more consistent
to either put "return true" there or change the !KASAN stub of this to
check for kfence objects or something like that? Honestly I think the
latter would be most appropriate, though then maybe the hook shouldn't
have "kasan" in its name...

Either way, I agree that the current situation wrt mempools and kfence
is inconsistent, but I think I should probably leave that as-is in my
series for now, and the kfence mempool issue can be addressed
separately afterwards? I also would like to avoid changing kfence
behavior as part of this patch.

If you want, I can add a comment above the "if (is_kfence_address())"
that notes the inconsistency?