Received-SPF: pass (google.com: domain of linux-kernel+bounces-40972-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240125094815.2041933-1-elver@google.com> <20240125094815.2041933-2-elver@google.com>
 <CA+fCnZc6L3t3AdQS1rjFCT0s6RpT+q4Z4GmctOveeaDJW0tBow@mail.gmail.com> <ZbPFUXNeENyuwync@elver.google.com>
In-Reply-To: <ZbPFUXNeENyuwync@elver.google.com>
From: Andrey Konovalov <andreyknvl@gmail.com>
Date: Sat, 27 Jan 2024 02:53:23 +0100
Message-ID: <CA+fCnZeOmHf-zjMnorXJQCyy3em9sMVS_uKaRUwZkbhVRVbRmg@mail.gmail.com>
Subject: Re: [PATCH 2/2] kasan: revert eviction of stack traces in generic mode
To: Marco Elver <elver@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, Alexander Potapenko <glider@google.com>, 
	Dmitry Vyukov <dvyukov@google.com>, Vlastimil Babka <vbabka@suse.cz>, 
	Andrey Ryabinin <ryabinin.a.a@gmail.com>, Vincenzo Frascino <vincenzo.frascino@arm.com>, 
	linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com, linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 26, 2024 at 3:44=E2=80=AFPM Marco Elver <elver@google.com> wrot=
e:
>
> On Thu, Jan 25, 2024 at 11:36PM +0100, Andrey Konovalov wrote:
> [...]
> >
> > Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
> >
> > But I'm wondering if we should also stop resetting metadata when the
> > object is fully freed (from quarantine or bypassing quarantine).
> >
> > With stack_depot_put, I had to put the stack handles on free, as
> > otherwise we would leak the stack depot references. And I also chose
> > to memset meta at that point, as its gets invalid anyway. But without
> > stack_depot_put, this is not required.
> >
> > Before the stack depot-related changes, the code was inconsistent in
> > this regard AFAICS: for quarantine, free meta was marked as invalid
> > via KASAN_SLAB_FREE but alloc meta was kept; for no quarantine, both
> > alloc and free meta were kept.
> >
> > So perhaps we can just keep both metas on full free. I.e. drop both
> > kasan_release_object_meta calls. This will go back to the old behavior
> > + keeping free meta for the quarantine case (I think there's no harm
> > in that). This will give better reporting for uaf-before-realloc bugs.
> >
> > WDYT?
>
> Yes, that makes sense.
>
> You mean this on top?
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index ad32803e34e9..0577db1d2c62 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -264,12 +264,6 @@ bool __kasan_slab_free(struct kmem_cache *cache, voi=
d *object,
>         if (kasan_quarantine_put(cache, object))
>                 return true;
>
> -       /*
> -        * If the object is not put into quarantine, it will likely be qu=
ickly
> -        * reallocated. Thus, release its metadata now.
> -        */
> -       kasan_release_object_meta(cache, object);
> -
>         /* Let slab put the object onto the freelist. */
>         return false;
>  }
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index 8bfb52b28c22..fc9cf1860efb 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -510,20 +510,6 @@ static void release_free_meta(const void *object, st=
ruct kasan_free_meta *meta)
>         *(u8 *)kasan_mem_to_shadow(object) =3D KASAN_SLAB_FREE;
>  }
>
> -void kasan_release_object_meta(struct kmem_cache *cache, const void *obj=
ect)
> -{
> -       struct kasan_alloc_meta *alloc_meta;
> -       struct kasan_free_meta *free_meta;
> -
> -       alloc_meta =3D kasan_get_alloc_meta(cache, object);
> -       if (alloc_meta)
> -               release_alloc_meta(alloc_meta);
> -
> -       free_meta =3D kasan_get_free_meta(cache, object);
> -       if (free_meta)
> -               release_free_meta(object, free_meta);
> -}
> -
>  size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object)
>  {
>         struct kasan_cache *info =3D &cache->kasan_info;
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 216ae0ef1e4b..fb2b9ac0659a 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -390,10 +390,8 @@ struct kasan_alloc_meta *kasan_get_alloc_meta(struct=
 kmem_cache *cache,
>  struct kasan_free_meta *kasan_get_free_meta(struct kmem_cache *cache,
>                                                 const void *object);
>  void kasan_init_object_meta(struct kmem_cache *cache, const void *object=
);
> -void kasan_release_object_meta(struct kmem_cache *cache, const void *obj=
ect);
>  #else
>  static inline void kasan_init_object_meta(struct kmem_cache *cache, cons=
t void *object) { }
> -static inline void kasan_release_object_meta(struct kmem_cache *cache, c=
onst void *object) { }
>  #endif
>
>  depot_stack_handle_t kasan_save_stack(gfp_t flags, depot_flags_t depot_f=
lags);
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 3ba02efb952a..a758c2e10703 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -145,8 +145,6 @@ static void qlink_free(struct qlist_node *qlink, stru=
ct kmem_cache *cache)
>         void *object =3D qlink_to_object(qlink, cache);
>         struct kasan_free_meta *free_meta =3D kasan_get_free_meta(cache, =
object);
>
> -       kasan_release_object_meta(cache, object);
> -
>         /*
>          * If init_on_free is enabled and KASAN's free metadata is stored=
 in
>          * the object, zero the metadata. Otherwise, the object's memory =
will

Please also add a comment saying something like "Keep per-object
metadata to allow KASAN print stack traces for
use-after-free-before-realloc bugs." to the places where you removed
kasan_release_object_meta.

Otherwise, looks good to me.