[PATCH 2/3] kasan: record and print the free track

[Walter Wu]

We add new KASAN_RCU_STACK_RECORD configuration option. It will move
free track from slub meta-data (struct kasan_alloc_meta) into freed object.
Because we hope this options doesn't enlarge slub meta-data size.

This option doesn't enlarge struct kasan_alloc_meta size.
- add two call_rcu() call stack into kasan_alloc_meta, size is 8 bytes.
- remove free track from kasan_alloc_meta, size is 8 bytes.

This option is only suitable for generic KASAN, because we move free track
into the freed object, so free track is valid information only when it
exists in quarantine. If the object is in-use state, then the KASAN report
doesn't print call_rcu() free track information.

[1]https://bugzilla.kernel.org/show_bug.cgi?id=198437

Signed-off-by: Walter Wu <walter...@mediatek.com>
Cc: Andrey Ryabinin <arya...@virtuozzo.com>
Cc: Dmitry Vyukov <dvy...@google.com>
Cc: Alexander Potapenko <gli...@google.com>
---
mm/kasan/common.c | 10 +++++++++-
mm/kasan/report.c | 24 +++++++++++++++++++++---
2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 32d422bdf127..13ec03e225a7 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -321,8 +321,15 @@ void kasan_record_callrcu(void *addr)
/* record last call_rcu() call stack */
alloc_info->rcu_free_stack[1] = save_stack(GFP_NOWAIT);
}
-#endif

+static void kasan_set_free_info(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ /* store free track into freed object */
+ set_track((struct kasan_track *)(object + BYTES_PER_WORD), GFP_NOWAIT);
+}
+
+#else
static void kasan_set_free_info(struct kmem_cache *cache,
void *object, u8 tag)
{
@@ -339,6 +346,7 @@ static void kasan_set_free_info(struct kmem_cache *cache,

set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
}
+#endif

void kasan_poison_slab(struct page *page)
{
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 7aaccc70b65b..f2b0c6b9dffa 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -175,8 +175,23 @@ static void kasan_print_rcu_free_stack(struct kasan_alloc_meta *alloc_info)
print_track(&free_track, "Last call_rcu() call stack", true);
pr_err("\n");
}
-#endif

+static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag, const void *addr)
+{
+ u8 *shadow_addr = (u8 *)kasan_mem_to_shadow(addr);
+
+ /*
+ * Only the freed object can get free track,
+ * because free track information is stored to freed object.
+ */
+ if (*shadow_addr == KASAN_KMALLOC_FREE)
+ return (struct kasan_track *)(object + BYTES_PER_WORD);
+ else
+ return NULL;
+}
+
+#else
static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
void *object, u8 tag, const void *addr)
{
@@ -196,6 +211,7 @@ static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,

return &alloc_meta->free_track[i];
}
+#endif

static void describe_object(struct kmem_cache *cache, void *object,
const void *addr, u8 tag)
@@ -208,8 +224,10 @@ static void describe_object(struct kmem_cache *cache, void *object,
print_track(&alloc_info->alloc_track, "Allocated", false);
pr_err("\n");
free_track = kasan_get_free_track(cache, object, tag, addr);
- print_track(free_track, "Freed", false);
- pr_err("\n");
+ if (free_track) {
+ print_track(free_track, "Freed", false);
+ pr_err("\n");
+ }
#ifdef CONFIG_KASAN_RCU_STACK_RECORD
kasan_print_rcu_free_stack(alloc_info);
#endif
--
2.18.0

[Dmitry Vyukov]

Humm... the other patch defines BYTES_PER_WORD as 4... I would assume
seeing 8 (or sizeof(long)) here. Why 4?
Have you tested all 4 modes (RCU/no-RCU x SLAB/SLUB)? As far as I
remember one of the allocators stored something in the object.

Also, does this work with objects with ctors and slabs destroyed by
rcu? kasan_track may smash other things in these cases.
Have you looked at the KASAN implementation when free_track was
removed? That may have useful details :)

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20200506052155.14515-1-walter-zh.wu%40mediatek.com.

[Walter Wu]

It should be a pointer size, maybe sizeof(long) makes more sense.

> Have you tested all 4 modes (RCU/no-RCU x SLAB/SLUB)? As far as I
> remember one of the allocators stored something in the object.

Good question, I only tested in RCU x SLUB, would you tell mew how do
no-RCU? I will test them in v2 pathset.

>
> Also, does this work with objects with ctors and slabs destroyed by
> rcu? kasan_track may smash other things in these cases.
> Have you looked at the KASAN implementation when free_track was
> removed? That may have useful details :)

Set free_track before put into quarantine, free_track should not have to
be removed, it only have to overwirte itself.

[Dmitry Vyukov]

I meant with CONFIG_KASAN_RCU_STACK_RECORD=y and with
CONFIG_KASAN_RCU_STACK_RECORD not set.
But if we drop CONFIG_KASAN_RCU_STACK_RECORD config, then it halves
the number of configurations to test ;)

[Walter Wu]

Ok, I have be tested by RCU xSLUB and no_RCUxSLUB, It is workable. So I
only miss this SLAB combination.

[Walter Wu]

In order not to enlarge slub meta-data size, so we move free track

from slub meta-data (struct kasan_alloc_meta) into freed object.

Modification of struct kasan_alloc_meta:
- add two call_rcu() stack into kasan_alloc_meta, size is 8 bytes.

- remove free track from kasan_alloc_meta, size is 8 bytes.

Because free track is stored in freed object, so that if it is an
allocation objects, then it will not have free track information in
KASAN report.

This feature is only suitable for generic KASAN, because we need to
know whether objects are allocation or free.
- if slub object is allocation state, it will not print free stack.
- if slub oeject is free state, it will print free stack.

[1]https://bugzilla.kernel.org/show_bug.cgi?id=198437

Signed-off-by: Walter Wu <walter...@mediatek.com>

Suggested-by: Dmitry Vyukov <dvy...@google.com>

Cc: Andrey Ryabinin <arya...@virtuozzo.com>
Cc: Dmitry Vyukov <dvy...@google.com>
Cc: Alexander Potapenko <gli...@google.com>
---

mm/kasan/common.c | 22 ++--------------------
mm/kasan/generic.c | 22 ++++++++++++++++++++++
mm/kasan/kasan.h | 4 ++++
mm/kasan/report.c | 28 +++++-----------------------
mm/kasan/tags.c | 37 +++++++++++++++++++++++++++++++++++++
5 files changed, 70 insertions(+), 43 deletions(-)

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 8bc618289bb1..47b53912f322 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -51,7 +51,7 @@ depot_stack_handle_t kasan_save_stack(gfp_t flags)
return stack_depot_save(entries, nr_entries, flags);
}

-static inline void set_track(struct kasan_track *track, gfp_t flags)
+void kasan_set_track(struct kasan_track *track, gfp_t flags)
{
track->pid = current->pid;
track->stack = kasan_save_stack(flags);
@@ -299,24 +299,6 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
return (void *)object + cache->kasan_info.free_meta_offset;
}

-
-static void kasan_set_free_info(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- u8 idx = 0;
-
- alloc_meta = get_alloc_info(cache, object);
-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
- idx = alloc_meta->free_track_idx;
- alloc_meta->free_pointer_tag[idx] = tag;
- alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
-#endif
-
- set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
-}
-
void kasan_poison_slab(struct page *page)
{
unsigned long i;
@@ -492,7 +474,7 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,
KASAN_KMALLOC_REDZONE);

if (cache->flags & SLAB_KASAN)
- set_track(&get_alloc_info(cache, object)->alloc_track, flags);
+ kasan_set_track(&get_alloc_info(cache, object)->alloc_track, flags);

return set_tag(object, tag);
}
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index b86880c338e2..dacff05a8107 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -354,3 +354,25 @@ struct kasan_track *kasan_get_aux_stack(struct kasan_alloc_meta *alloc_info,
return container_of(&alloc_info->rcu_stack[idx],
struct kasan_track, stack);
}
+
+void kasan_set_free_info(struct kmem_cache *cache,

+ void *object, u8 tag)
+{
+ /* store free track into freed object */

+ kasan_set_track((struct kasan_track *)(object + SIZEOF_PTR), GFP_NOWAIT);
+}
+
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,

+ void *object, u8 tag, const void *addr)
+{
+ u8 *shadow_addr = (u8 *)kasan_mem_to_shadow(addr);
+
+ /*
+ * Only the freed object can get free track,
+ * because free track information is stored to freed object.
+ */
+ if (*shadow_addr == KASAN_KMALLOC_FREE)

+ return (struct kasan_track *)(object + SIZEOF_PTR);

+ else
+ return NULL;
+}

diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 1cc1fb7b0de3..f88d13f86ed3 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -173,6 +173,10 @@ void kasan_report_invalid_free(void *object, unsigned long ip);
struct page *kasan_addr_to_page(const void *addr);

depot_stack_handle_t kasan_save_stack(gfp_t flags);
+void kasan_set_track(struct kasan_track *track, gfp_t flags);
+void kasan_set_free_info(struct kmem_cache *cache, void *object, u8 tag);
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag, const void *addr);

#if defined(CONFIG_KASAN_GENERIC) && \
(defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index f16a1a210815..51813f02992c 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -163,26 +163,6 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
(void *)(object_addr + cache->object_size));
}

-static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- int i = 0;
-
- alloc_meta = get_alloc_info(cache, object);
-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
- for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
- if (alloc_meta->free_pointer_tag[i] == tag)
- break;
- }
- if (i == KASAN_NR_FREE_STACKS)
- i = alloc_meta->free_track_idx;
-#endif
-
- return &alloc_meta->free_track[i];
-}
-

static void describe_object(struct kmem_cache *cache, void *object,
const void *addr, u8 tag)
{

@@ -193,9 +173,11 @@ static void describe_object(struct kmem_cache *cache, void *object,

print_track(&alloc_info->alloc_track, "Allocated", false);
pr_err("\n");

- free_track = kasan_get_free_track(cache, object, tag);

- print_track(free_track, "Freed", false);
- pr_err("\n");

+ free_track = kasan_get_free_track(cache, object, tag, addr);

+ if (free_track) {
+ print_track(free_track, "Freed", false);
+ pr_err("\n");
+ }

if (IS_ENABLED(CONFIG_KASAN_GENERIC)) {
free_track = kasan_get_aux_stack(alloc_info, 0);
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index 25b7734e7013..30a27f8c1e6e 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -162,3 +162,40 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size)
kasan_poison_shadow((void *)addr, size, tag);
}
EXPORT_SYMBOL(__hwasan_tag_memory);
+
+void kasan_set_free_info(struct kmem_cache *cache,

+ void *object, u8 tag)
+{

+ struct kasan_alloc_meta *alloc_meta;
+ u8 idx = 0;
+
+ alloc_meta = get_alloc_info(cache, object);
+
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ idx = alloc_meta->free_track_idx;
+ alloc_meta->free_pointer_tag[idx] = tag;
+ alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
+#endif
+
+ kasan_set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
+}
+
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,

+ void *object, u8 tag, const void *addr)
+{

+ struct kasan_alloc_meta *alloc_meta;
+ int i = 0;
+
+ alloc_meta = get_alloc_info(cache, object);
+
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
+ if (alloc_meta->free_pointer_tag[i] == tag)
+ break;
+ }
+ if (i == KASAN_NR_FREE_STACKS)
+ i = alloc_meta->free_track_idx;
+#endif
+
+ return &alloc_meta->free_track[i];
+}
--
2.18.0

[Walter Wu]

Move free track from slub alloc meta-data to slub free meta-data in
order to make struct kasan_free_meta size is 16 bytes. It is a good
size because it is the minimal redzone size and a good number of
alignment.

For free track in generic KASAN, we do the modification in struct
kasan_alloc_meta and kasan_free_meta:
- remove free track from kasan_alloc_meta.
- add free track into kasan_free_meta.

[1]https://bugzilla.kernel.org/show_bug.cgi?id=198437

Signed-off-by: Walter Wu <walter...@mediatek.com>
Suggested-by: Dmitry Vyukov <dvy...@google.com>
Cc: Andrey Ryabinin <arya...@virtuozzo.com>
Cc: Dmitry Vyukov <dvy...@google.com>
Cc: Alexander Potapenko <gli...@google.com>
---

mm/kasan/common.c | 33 ++++++++++-----------------------
mm/kasan/generic.c | 18 ++++++++++++++++++
mm/kasan/kasan.h | 7 +++++++
mm/kasan/report.c | 20 --------------------
mm/kasan/tags.c | 37 +++++++++++++++++++++++++++++++++++++
5 files changed, 72 insertions(+), 43 deletions(-)

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 8bc618289bb1..6500bc2bb70c 100644

--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -51,7 +51,7 @@ depot_stack_handle_t kasan_save_stack(gfp_t flags)
return stack_depot_save(entries, nr_entries, flags);
}

-static inline void set_track(struct kasan_track *track, gfp_t flags)
+void kasan_set_track(struct kasan_track *track, gfp_t flags)
{
track->pid = current->pid;
track->stack = kasan_save_stack(flags);

@@ -249,9 +249,7 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
*size += sizeof(struct kasan_alloc_meta);

/* Add free meta. */
- if (IS_ENABLED(CONFIG_KASAN_GENERIC) &&
- (cache->flags & SLAB_TYPESAFE_BY_RCU || cache->ctor ||
- cache->object_size < sizeof(struct kasan_free_meta))) {
+ if (IS_ENABLED(CONFIG_KASAN_GENERIC)) {
cache->kasan_info.free_meta_offset = *size;
*size += sizeof(struct kasan_free_meta);
}
@@ -299,24 +297,6 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,

return (void *)object + cache->kasan_info.free_meta_offset;
}

-
-static void kasan_set_free_info(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- u8 idx = 0;
-
- alloc_meta = get_alloc_info(cache, object);
-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
- idx = alloc_meta->free_track_idx;
- alloc_meta->free_pointer_tag[idx] = tag;
- alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
-#endif
-
- set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
-}
-
void kasan_poison_slab(struct page *page)
{
unsigned long i;

@@ -396,6 +376,13 @@ void * __must_check kasan_init_slab_obj(struct kmem_cache *cache,
alloc_info = get_alloc_info(cache, object);
__memset(alloc_info, 0, sizeof(*alloc_info));

+ if (IS_ENABLED(CONFIG_KASAN_GENERIC)) {
+ struct kasan_free_meta *free_info;
+
+ free_info = get_free_info(cache, object);
+ __memset(free_info, 0, sizeof(*free_info));
+ }
+
if (IS_ENABLED(CONFIG_KASAN_SW_TAGS))
object = set_tag(object,
assign_tag(cache, object, true, false));
@@ -492,7 +479,7 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,

KASAN_KMALLOC_REDZONE);

if (cache->flags & SLAB_KASAN)
- set_track(&get_alloc_info(cache, object)->alloc_track, flags);
+ kasan_set_track(&get_alloc_info(cache, object)->alloc_track, flags);

return set_tag(object, tag);
}
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c

index 78d8e0a75a8a..988bc095b738 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -345,3 +345,21 @@ void kasan_record_aux_stack(void *addr)
alloc_info->rcu_stack[1] = alloc_info->rcu_stack[0];
alloc_info->rcu_stack[0] = kasan_save_stack(GFP_NOWAIT);

}
+
+void kasan_set_free_info(struct kmem_cache *cache,
+ void *object, u8 tag)
+{

+ struct kasan_free_meta *free_meta;
+
+ free_meta = get_free_info(cache, object);
+ kasan_set_track(&free_meta->free_track, GFP_NOWAIT);

+}
+
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,

+ void *object, u8 tag)
+{
+ struct kasan_free_meta *free_meta;
+
+ free_meta = get_free_info(cache, object);
+ return &free_meta->free_track;

+}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h

index 870c5dd07756..87ee3626b8b0 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -127,6 +127,9 @@ struct kasan_free_meta {
* Otherwise it might be used for the allocator freelist.
*/
struct qlist_node quarantine_link;
+#ifdef CONFIG_KASAN_GENERIC
+ struct kasan_track free_track;
+#endif
};

struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
@@ -168,6 +171,10 @@ void kasan_report_invalid_free(void *object, unsigned long ip);

struct page *kasan_addr_to_page(const void *addr);

depot_stack_handle_t kasan_save_stack(gfp_t flags);
+void kasan_set_track(struct kasan_track *track, gfp_t flags);
+void kasan_set_free_info(struct kmem_cache *cache, void *object, u8 tag);
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,

+ void *object, u8 tag);

#if defined(CONFIG_KASAN_GENERIC) && \
(defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
diff --git a/mm/kasan/report.c b/mm/kasan/report.c

index 5ee66cf7e27c..7e9f9f6d5e85 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -159,26 +159,6 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,

(void *)(object_addr + cache->object_size));
}

-static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- int i = 0;
-
- alloc_meta = get_alloc_info(cache, object);
-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
- for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
- if (alloc_meta->free_pointer_tag[i] == tag)
- break;
- }
- if (i == KASAN_NR_FREE_STACKS)
- i = alloc_meta->free_track_idx;
-#endif
-
- return &alloc_meta->free_track[i];
-}
-

#ifdef CONFIG_KASAN_GENERIC
static void print_stack(depot_stack_handle_t stack)
{
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index 25b7734e7013..201dee5d6ae0 100644

[Dmitry Vyukov]

Why do we need to increase object size unconditionally?
We only store info in free track when the object is free, so I would
assume we still can generally overlap free track and the object
itself. We store free track at the same time we use the quarantine
link, and the quarantine link was overlapped with the object just
fine.
With this change we indeed increase object size, which we do not want
in general.

If we overlap free track with object, this will not be needed as well, right?

[Walter Wu]

If it doesn't add free meta, but we always store free track into the
object, Is it safe?

Should we not consider those objects which have adding free meta? If
they exist, then we should init their meta data when object re-allocate.

struct kasan_free_meta {
struct qlist_node quarantine_link;
struct kasan_track free_track;
};

[Walter Wu]

I thought about it, I think you are right, because the free track must
be stored when object is free, so even don't clean this meta data. It
doesn't matter.

Thanks for your review. If there are no other problems, I will send next
patch.

Thanks.

[Walter Wu]

Move free track from slub alloc meta-data to slub free meta-data in
order to make struct kasan_free_meta size is 16 bytes. It is a good
size because it is the minimal redzone size and a good number of
alignment.

For free track in generic KASAN, we do the modification in struct
kasan_alloc_meta and kasan_free_meta:
- remove free track from kasan_alloc_meta.
- add free track into kasan_free_meta.

[1]https://bugzilla.kernel.org/show_bug.cgi?id=198437

Signed-off-by: Walter Wu <walter...@mediatek.com>
Suggested-by: Dmitry Vyukov <dvy...@google.com>
Cc: Andrey Ryabinin <arya...@virtuozzo.com>
Cc: Dmitry Vyukov <dvy...@google.com>
Cc: Alexander Potapenko <gli...@google.com>
---

mm/kasan/common.c | 22 ++--------------------

mm/kasan/generic.c | 18 ++++++++++++++++++
mm/kasan/kasan.h | 7 +++++++
mm/kasan/report.c | 20 --------------------
mm/kasan/tags.c | 37 +++++++++++++++++++++++++++++++++++++

5 files changed, 64 insertions(+), 40 deletions(-)

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 8bc618289bb1..47b53912f322 100644

--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -51,7 +51,7 @@ depot_stack_handle_t kasan_save_stack(gfp_t flags)
return stack_depot_save(entries, nr_entries, flags);
}

-static inline void set_track(struct kasan_track *track, gfp_t flags)
+void kasan_set_track(struct kasan_track *track, gfp_t flags)
{
track->pid = current->pid;
track->stack = kasan_save_stack(flags);

@@ -299,24 +299,6 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,

return (void *)object + cache->kasan_info.free_meta_offset;
}

-
-static void kasan_set_free_info(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- u8 idx = 0;
-
- alloc_meta = get_alloc_info(cache, object);
-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
- idx = alloc_meta->free_track_idx;
- alloc_meta->free_pointer_tag[idx] = tag;
- alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
-#endif
-
- set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
-}
-
void kasan_poison_slab(struct page *page)
{
unsigned long i;

@@ -492,7 +474,7 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,

KASAN_KMALLOC_REDZONE);

if (cache->flags & SLAB_KASAN)
- set_track(&get_alloc_info(cache, object)->alloc_track, flags);
+ kasan_set_track(&get_alloc_info(cache, object)->alloc_track, flags);

return set_tag(object, tag);
}
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c

index 3372bdcaf92a..763d8a13e0ac 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -344,3 +344,21 @@ void kasan_record_aux_stack(void *addr)
alloc_info->aux_stack[1] = alloc_info->aux_stack[0];
alloc_info->aux_stack[0] = kasan_save_stack(GFP_NOWAIT);

}
+
+void kasan_set_free_info(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ struct kasan_free_meta *free_meta;
+
+ free_meta = get_free_info(cache, object);
+ kasan_set_track(&free_meta->free_track, GFP_NOWAIT);
+}
+
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ struct kasan_free_meta *free_meta;
+
+ free_meta = get_free_info(cache, object);
+ return &free_meta->free_track;
+}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h

index a7391bc83070..ad897ec36545 100644

--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -127,6 +127,9 @@ struct kasan_free_meta {
* Otherwise it might be used for the allocator freelist.
*/
struct qlist_node quarantine_link;
+#ifdef CONFIG_KASAN_GENERIC
+ struct kasan_track free_track;
+#endif
};

struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
@@ -168,6 +171,10 @@ void kasan_report_invalid_free(void *object, unsigned long ip);
struct page *kasan_addr_to_page(const void *addr);

depot_stack_handle_t kasan_save_stack(gfp_t flags);
+void kasan_set_track(struct kasan_track *track, gfp_t flags);
+void kasan_set_free_info(struct kmem_cache *cache, void *object, u8 tag);
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag);

#if defined(CONFIG_KASAN_GENERIC) && \
(defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
diff --git a/mm/kasan/report.c b/mm/kasan/report.c

index 6f8f2bf8f53b..96d2657fe70f 100644

[Dmitry Vyukov]

Hi Walter,

FTR I've uploaded this for review purposes here:
https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/2458

Diff from the previous version is available as:
https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/2458/1..2

I've tested this locally and with syzkaller. This is 🔥🔥🔥:

[ 80.583021][ C3] Freed by task 0:
[ 80.583480][ C3] kasan_save_stack+0x1b/0x40 mm/kasan/common.c:49
[ 80.584056][ C3] kasan_set_track+0x1c/0x30 mm/kasan/common.c:57
[ 80.584617][ C3] kasan_set_free_info+0x1b/0x30 mm/kasan/generic.c:354
[ 80.585221][ C3] __kasan_slab_free+0xd8/0x120 mm/kasan/common.c:438
[ 80.585814][ C3] __cache_free mm/slab.c:3426 [inline]
[ 80.585814][ C3] kfree+0x10b/0x2b0 mm/slab.c:3757
[ 80.586291][ C3] kasan_rcu_reclaim+0x16/0x43 [test_kasan]
[ 80.587009][ C3] rcu_do_batch kernel/rcu/tree.c:2207 [inline]
[ 80.587009][ C3] rcu_core+0x59f/0x1370 kernel/rcu/tree.c:2434
[ 80.587537][ C3] __do_softirq+0x26c/0x9fa kernel/softirq.c:292
[ 80.588085][ C3]
[ 80.588367][ C3] Last one call_rcu() call stack:
[ 80.589052][ C3] kasan_save_stack+0x1b/0x40 mm/kasan/common.c:49
[ 80.589622][ C3] kasan_record_aux_stack+0x82/0xb0 mm/kasan/generic.c:345
[ 80.590254][ C3] __call_rcu kernel/rcu/tree.c:2672 [inline]
[ 80.590254][ C3] call_rcu+0x14f/0x7f0 kernel/rcu/tree.c:2746
[ 80.590782][ C3] kasan_rcu_uaf+0xe4/0xeb [test_kasan]
[ 80.591697][ C3] kmalloc_tests_init+0xbc/0x1097 [test_kasan]
[ 80.592900][ C3] do_one_initcall+0x10a/0x7d0 init/main.c:1196
[ 80.593494][ C3] do_init_module+0x1e6/0x6d0 kernel/module.c:3539
[ 80.594066][ C3] load_module+0x7464/0x9450 kernel/module.c:3890
[ 80.594626][ C3] __do_sys_init_module+0x1e3/0x220 kernel/module.c:3953
[ 80.595265][ C3] do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
[ 80.595822][ C3] entry_SYSCALL_64_after_hwframe+0x49/0xb3


Overall this looks very good to me.
But there is one aspect that bothers me. In the previous patch you had
code that returned NULL from kasan_get_free_track() if the object is
live (which means free meta is not available, it's occupied by object
data). Now you dropped that code, but I think we still need it.
Otherwise we cast user object data to free meta and print the free
stack/pid from whatever garbage is there. This may lead to very
confusing output and potentially to crashes in stackdepot.

What do you think about this patch on top of your patches?
https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/2478
This way we very precisely mark the period of time when the object has
free track live and set.
If it looks good to you, feel free to incorporate it into your series.

[Walter Wu]

Yes, I totally agree with you. In the previous email I thought that
there is a problem with free track, but I did not point it out. Thank
you for pointing this problem. As you mentioned, we should fix it.

> What do you think about this patch on top of your patches?
> https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/2478
> This way we very precisely mark the period of time when the object has
> free track live and set.
> If it looks good to you, feel free to incorporate it into your series.
>

Thank you for providing good idea solution.

I saw this patch, that is a great patch. I think it can fix the issue
which has garbage stack. it should work as described below.

1). When object is live, then don't print free stack.
2). When object is NOT alive, after free object:
2a). when object is in quarantine, then it can print free stack
2b). when object is NOT in quarantine, then it can NOT print free stack.

I have a question about 2), why we don't directly use
KASAN_KMALLOC_FREE? if we directly use it, then 2b) can print free
stack? 2b) may has use-after-free? so that it may need free stack.

[Dmitry Vyukov]

We can't use KASAN_KMALLOC_FREE because of this part:

static bool __kasan_slab_free(struct kmem_cache *cache, void *object,
unsigned long ip, bool quarantine)
{
...
kasan_poison_shadow(object, rounded_up_size, KASAN_KMALLOC_FREE);

if ((IS_ENABLED(CONFIG_KASAN_GENERIC) && !quarantine) ||
unlikely(!(cache->flags & SLAB_KASAN)))
return false;

kasan_set_free_info(cache, object, tag);
...


We may set KASAN_KMALLOC_FREE, but not set the track (or even have
memory for the track!).
The object may not have free meta allocated at all, e.g. very large
object with ctor (no place to store meta), or it may be in a mempool:
https://elixir.bootlin.com/linux/v5.7-rc6/source/mm/mempool.c#L109
and mempool may be using the object memory itself (for its own next
link or something).

KASAN_KMALLOC_FREETRACK very explicitly tracks the exact condition we
want: we have meta info live now and we have free track set.

[Walter Wu]

Ok, I see. When return false, then the shadow memory content has
KASAN_KMALLOC_FREE, but it doesn't set free stack, so that we need to
avoid this situation. Thank for you reminder.

[Walter Wu]

Yes, as you said, it is needed by this change.

[Walter Wu]

About 2b), I see another question. When do qlink_free(), it will be
written KASAN_KMALLOC_FREE from KASAN_KMALLOC_FREETRACK? if we don't
write shadow memory, it is still KASAN_KMALLOC_FREETRACK, then 2b) will
have free stack? Because I see you add KASAN_KMALLOC_FREETRACK to get
use-after-free in get_shadow_bug_type(). so should it not write
KASAN_KMALLOC_FREE?

[Dmitry Vyukov]

It may or may not work.
The potential problem is that when qlink_free calls ___cache_free,
slab/slub may start using object memory for its own purposes, e.g.
store the next link. This next link may overwrite part of free meta.
It actually may work because the slab/slib next link is likely to
overlap with kasan_free_meta.quarantine_link only. And we may have
kasan_free_meta.free_track intact while KASAN_KMALLOC_FREE is set. But
this needs careful checking for both slab and slub and if they may use
more than 1 word in some configurations.

[Walter Wu]

This problem looks like existing, even without this change? currently
KASAN may get wrong free stack?

Regardless of whether the shadow memory content is
KASAN_KMALLOC_FREETRACK or KASAN_KMALLOC_FREE, it may have this problem?
But because of kasan_get_free_track() have conditions to get free track,
so that if shadow memory content is KASAN_KMALLOC_FREE, then it will
avoid this problem and always print right free stack.

[Dmitry Vyukov]

No, we should not have this problem now. Currently free track is
stored in alloc meta. Alloc meta does not overlap with the object.
It's only free meta that overlaps with the object and slab metadata at
different periods of the block lifetime. Schematically what we have
is:

struct block_t {
alloc_meta kasan_alloc_meta;
union {
user_data char[N];
slab_meta slab_meta;
free_meta kasan_free_meta;
};
}

free_meta shared storage space with 2 other things.

> Regardless of whether the shadow memory content is
> KASAN_KMALLOC_FREETRACK or KASAN_KMALLOC_FREE, it may have this problem?

KASAN_KMALLOC_FREETRACK is set only when nobody else uses the storage.

[Walter Wu]

Ah...I forget it is stored in alloc mata, Yes, it should not have this
problem.

Thanks for your detailed explanation

> > Regardless of whether the shadow memory content is
> > KASAN_KMALLOC_FREETRACK or KASAN_KMALLOC_FREE, it may have this problem?
>
> KASAN_KMALLOC_FREETRACK is set only when nobody else uses the storage.
>

Ok, I will use KASAN_KMALLOC_FREE. If you have any concerns, please tell me.
Thanks.

[Dmitry Vyukov]

You mean KASAN_KMALLOC_FREETRACK?

Or, you checked that using KASAN_KMALLOC_FREE is safe and will not
cause any bad overlap?

[Walter Wu]

Yes, sorry, I made you misunderstand what I meant. I mean that qlink_free()
write KASAN_KMALLOC_FREE.

> Or, you checked that using KASAN_KMALLOC_FREE is safe and will not
> cause any bad overlap?
>

This item you refer to, maybe it can be done in the future.

[Dmitry Vyukov]

Good.

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/1589974955.3182.8.camel%40mtksdccf07.

[Walter Wu]

Move free track from kasan_alloc_meta to kasan_free_meta in order
to make struct kasan_alloc_meta and kasan_free_meta size are both

16 bytes. It is a good size because it is the minimal redzone size
and a good number of alignment.

For free track, we make some modifications as shown below:
1) Remove the free_track from struct kasan_alloc_meta.
2) Add the free_track into struct kasan_free_meta.
3) Add a macro KASAN_KMALLOC_FREETRACK in order to check whether
print free stack in KASAN report.

[1]https://bugzilla.kernel.org/show_bug.cgi?id=198437

Signed-off-by: Walter Wu <walter...@mediatek.com>
Suggested-by: Dmitry Vyukov <dvy...@google.com>

Co-developed-by: Dmitry Vyukov <dvy...@google.com>

Cc: Andrey Ryabinin <arya...@virtuozzo.com>
Cc: Dmitry Vyukov <dvy...@google.com>
Cc: Alexander Potapenko <gli...@google.com>
---
mm/kasan/common.c | 22 ++--------------------

mm/kasan/generic.c | 22 ++++++++++++++++++++++
mm/kasan/generic_report.c | 1 +
mm/kasan/kasan.h | 13 +++++++++++--
mm/kasan/quarantine.c | 1 +
mm/kasan/report.c | 26 ++++----------------------
mm/kasan/tags.c | 37 +++++++++++++++++++++++++++++++++++++
7 files changed, 78 insertions(+), 44 deletions(-)

index 8acf48882ba2..4b3cbad7431b 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -346,3 +346,25 @@ void kasan_record_aux_stack(void *addr)

alloc_info->aux_stack[1] = alloc_info->aux_stack[0];
alloc_info->aux_stack[0] = kasan_save_stack(GFP_NOWAIT);
}
+
+void kasan_set_free_info(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ struct kasan_free_meta *free_meta;
+
+ free_meta = get_free_info(cache, object);
+ kasan_set_track(&free_meta->free_track, GFP_NOWAIT);
+

+ /*
+ * the object was freed and has free track set
+ */
+ *(u8 *)kasan_mem_to_shadow(object) = KASAN_KMALLOC_FREETRACK;

+}
+
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag)
+{

+ if (*(u8 *)kasan_mem_to_shadow(object) != KASAN_KMALLOC_FREETRACK)
+ return NULL;
+ return &get_free_info(cache, object)->free_track;
+}
diff --git a/mm/kasan/generic_report.c b/mm/kasan/generic_report.c
index e200acb2d292..a38c7a9e192a 100644
--- a/mm/kasan/generic_report.c
+++ b/mm/kasan/generic_report.c
@@ -80,6 +80,7 @@ static const char *get_shadow_bug_type(struct kasan_access_info *info)
break;
case KASAN_FREE_PAGE:
case KASAN_KMALLOC_FREE:
+ case KASAN_KMALLOC_FREETRACK:
bug_type = "use-after-free";
break;
case KASAN_ALLOCA_LEFT:
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index a7391bc83070..ef655a1c6e15 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -17,15 +17,17 @@
#define KASAN_PAGE_REDZONE 0xFE /* redzone for kmalloc_large allocations */
#define KASAN_KMALLOC_REDZONE 0xFC /* redzone inside slub object */
#define KASAN_KMALLOC_FREE 0xFB /* object was freed (kmem_cache_free/kfree) */
+#define KASAN_KMALLOC_FREETRACK 0xFA /* object was freed and has free track set */
#else
#define KASAN_FREE_PAGE KASAN_TAG_INVALID
#define KASAN_PAGE_REDZONE KASAN_TAG_INVALID
#define KASAN_KMALLOC_REDZONE KASAN_TAG_INVALID
#define KASAN_KMALLOC_FREE KASAN_TAG_INVALID
+#define KASAN_KMALLOC_FREETRACK KASAN_TAG_INVALID
#endif

-#define KASAN_GLOBAL_REDZONE 0xFA /* redzone for global variable */
-#define KASAN_VMALLOC_INVALID 0xF9 /* unallocated space in vmapped page */
+#define KASAN_GLOBAL_REDZONE 0xF9 /* redzone for global variable */
+#define KASAN_VMALLOC_INVALID 0xF8 /* unallocated space in vmapped page */

/*
* Stack redzone shadow values
@@ -127,6 +129,9 @@ struct kasan_free_meta {

* Otherwise it might be used for the allocator freelist.
*/
struct qlist_node quarantine_link;
+#ifdef CONFIG_KASAN_GENERIC
+ struct kasan_track free_track;
+#endif
};

struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,

@@ -168,6 +173,10 @@ void kasan_report_invalid_free(void *object, unsigned long ip);

struct page *kasan_addr_to_page(const void *addr);

depot_stack_handle_t kasan_save_stack(gfp_t flags);
+void kasan_set_track(struct kasan_track *track, gfp_t flags);
+void kasan_set_free_info(struct kmem_cache *cache, void *object, u8 tag);
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag);

#if defined(CONFIG_KASAN_GENERIC) && \
(defined(CONFIG_SLAB) || defined(CONFIG_SLUB))

diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 978bc4a3eb51..4c5375810449 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -145,6 +145,7 @@ static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
if (IS_ENABLED(CONFIG_SLAB))
local_irq_save(flags);

+ *(u8 *)kasan_mem_to_shadow(object) = KASAN_KMALLOC_FREE;
___cache_free(cache, object, _THIS_IP_);

if (IS_ENABLED(CONFIG_SLAB))
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 29a801d5cd74..94b76a1df976 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -170,26 +170,6 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,

(void *)(object_addr + cache->object_size));
}

-static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- int i = 0;
-
- alloc_meta = get_alloc_info(cache, object);
-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
- for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
- if (alloc_meta->free_pointer_tag[i] == tag)
- break;
- }
- if (i == KASAN_NR_FREE_STACKS)
- i = alloc_meta->free_track_idx;
-#endif
-
- return &alloc_meta->free_track[i];
-}
-

static void describe_object(struct kmem_cache *cache, void *object,
const void *addr, u8 tag)
{

@@ -201,8 +181,10 @@ static void describe_object(struct kmem_cache *cache, void *object,
print_track(&alloc_info->alloc_track, "Allocated");
pr_err("\n");

free_track = kasan_get_free_track(cache, object, tag);

- print_track(free_track, "Freed");
- pr_err("\n");
+ if (free_track) {
+ print_track(free_track, "Freed");

+ pr_err("\n");
+ }

#ifdef CONFIG_KASAN_GENERIC
if (alloc_info->aux_stack[0]) {

--
2.18.0

[Walter Wu]

Move free track from kasan_alloc_meta to kasan_free_meta in order
to make struct kasan_alloc_meta and kasan_free_meta size are both
16 bytes. It is a good size because it is the minimal redzone size
and a good number of alignment.

For free track, we make some modifications as shown below:
1) Remove the free_track from struct kasan_alloc_meta.
2) Add the free_track into struct kasan_free_meta.
3) Add a macro KASAN_KMALLOC_FREETRACK in order to check whether

it can print free stack in KASAN report.

index 2421a4bd9227..fed3c8fdfd25 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -164,26 +164,6 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,

(void *)(object_addr + cache->object_size));
}

-static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- int i = 0;
-
- alloc_meta = get_alloc_info(cache, object);
-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
- for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
- if (alloc_meta->free_pointer_tag[i] == tag)
- break;
- }
- if (i == KASAN_NR_FREE_STACKS)
- i = alloc_meta->free_track_idx;
-#endif
-
- return &alloc_meta->free_track[i];
-}
-
static void describe_object(struct kmem_cache *cache, void *object,
const void *addr, u8 tag)
{

@@ -195,8 +175,10 @@ static void describe_object(struct kmem_cache *cache, void *object,

[Dmitry Vyukov]

On Fri, May 22, 2020 at 4:01 AM Walter Wu <walter...@mediatek.com> wrote:
>
> Move free track from kasan_alloc_meta to kasan_free_meta in order
> to make struct kasan_alloc_meta and kasan_free_meta size are both
> 16 bytes. It is a good size because it is the minimal redzone size
> and a good number of alignment.
>
> For free track, we make some modifications as shown below:
> 1) Remove the free_track from struct kasan_alloc_meta.
> 2) Add the free_track into struct kasan_free_meta.
> 3) Add a macro KASAN_KMALLOC_FREETRACK in order to check whether
> it can print free stack in KASAN report.
>
> [1]https://bugzilla.kernel.org/show_bug.cgi?id=198437

Reviewed-and-tested-by: Dmitry Vyukov <dvy...@google.com>

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20200522020127.23335-1-walter-zh.wu%40mediatek.com.

[Walter Wu]

Move free track from kasan_alloc_meta to kasan_free_meta in order
to make struct kasan_alloc_meta and kasan_free_meta size are both
16 bytes. It is a good size because it is the minimal redzone size
and a good number of alignment.

For free track, we make some modifications as shown below:
1) Remove the free_track from struct kasan_alloc_meta.
2) Add the free_track into struct kasan_free_meta.
3) Add a macro KASAN_KMALLOC_FREETRACK in order to check whether
it can print free stack in KASAN report.

[1]https://bugzilla.kernel.org/show_bug.cgi?id=198437

Signed-off-by: Walter Wu <walter...@mediatek.com>
Suggested-by: Dmitry Vyukov <dvy...@google.com>
Co-developed-by: Dmitry Vyukov <dvy...@google.com>

Reviewed-and-tested-by: Dmitry Vyukov <dvy...@google.com>
Reviewed-by: Andrey Konovalov <andre...@google.com>
Cc: Andrey Ryabinin <arya...@virtuozzo.com>