diff mbox series

[v8,09/20] kasan: preassign tags to objects with ctors or SLAB_TYPESAFE_BY_RCU

Message ID d74e710797323db0e43f047ea698fbc85060fc57.1537383101.git.andreyknvl@google.com (mailing list archive)
State New, archived
Headers show
Series kasan: add software tag-based mode for arm64 | expand

Commit Message

Andrey Konovalov Sept. 19, 2018, 6:54 p.m. UTC
An object constructor can initialize pointers within this objects based on
the address of the object. Since the object address might be tagged, we
need to assign a tag before calling constructor.

The implemented approach is to assign tags to objects with constructors
when a slab is allocated and call constructors once as usual. The
downside is that such object would always have the same tag when it is
reallocated, so we won't catch use-after-frees on it.

Also pressign tags for objects from SLAB_TYPESAFE_BY_RCU caches, since
they can be validy accessed after having been freed.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 mm/slab.c |  2 +-
 mm/slub.c | 24 ++++++++++++++----------
 2 files changed, 15 insertions(+), 11 deletions(-)

Comments

Dmitry Vyukov Sept. 21, 2018, 11:25 a.m. UTC | #1
On Wed, Sep 19, 2018 at 8:54 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> An object constructor can initialize pointers within this objects based on
> the address of the object. Since the object address might be tagged, we
> need to assign a tag before calling constructor.
>
> The implemented approach is to assign tags to objects with constructors
> when a slab is allocated and call constructors once as usual. The
> downside is that such object would always have the same tag when it is
> reallocated, so we won't catch use-after-frees on it.
>
> Also pressign tags for objects from SLAB_TYPESAFE_BY_RCU caches, since
> they can be validy accessed after having been freed.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  mm/slab.c |  2 +-
>  mm/slub.c | 24 ++++++++++++++----------
>  2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index 6fdca9ec2ea4..fe0ddf08aa2c 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2574,7 +2574,7 @@ static void cache_init_objs(struct kmem_cache *cachep,
>
>         for (i = 0; i < cachep->num; i++) {
>                 objp = index_to_obj(cachep, page, i);
> -               kasan_init_slab_obj(cachep, objp);
> +               objp = kasan_init_slab_obj(cachep, objp);
>
>                 /* constructor could break poison info */
>                 if (DEBUG == 0 && cachep->ctor) {
> diff --git a/mm/slub.c b/mm/slub.c
> index c4d5f4442ff1..75fc76e42a1e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1413,16 +1413,17 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
>  #endif
>  }
>
> -static void setup_object(struct kmem_cache *s, struct page *page,
> +static void *setup_object(struct kmem_cache *s, struct page *page,
>                                 void *object)
>  {
>         setup_object_debug(s, page, object);
> -       kasan_init_slab_obj(s, object);
> +       object = kasan_init_slab_obj(s, object);
>         if (unlikely(s->ctor)) {
>                 kasan_unpoison_object_data(s, object);
>                 s->ctor(object);
>                 kasan_poison_object_data(s, object);
>         }
> +       return object;
>  }
>
>  /*
> @@ -1530,16 +1531,16 @@ static bool shuffle_freelist(struct kmem_cache *s, struct page *page)
>         /* First entry is used as the base of the freelist */
>         cur = next_freelist_entry(s, page, &pos, start, page_limit,
>                                 freelist_count);
> +       cur = setup_object(s, page, cur);
>         page->freelist = cur;
>
>         for (idx = 1; idx < page->objects; idx++) {
> -               setup_object(s, page, cur);
>                 next = next_freelist_entry(s, page, &pos, start, page_limit,
>                         freelist_count);
> +               next = setup_object(s, page, next);
>                 set_freepointer(s, cur, next);
>                 cur = next;
>         }
> -       setup_object(s, page, cur);
>         set_freepointer(s, cur, NULL);
>
>         return true;
> @@ -1561,7 +1562,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
>         struct page *page;
>         struct kmem_cache_order_objects oo = s->oo;
>         gfp_t alloc_gfp;
> -       void *start, *p;
> +       void *start, *p, *next;
>         int idx, order;
>         bool shuffle;
>
> @@ -1613,13 +1614,16 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
>
>         if (!shuffle) {
>                 for_each_object_idx(p, idx, s, start, page->objects) {
> -                       setup_object(s, page, p);
> -                       if (likely(idx < page->objects))
> -                               set_freepointer(s, p, p + s->size);
> -                       else
> +                       if (likely(idx < page->objects)) {
> +                               next = p + s->size;
> +                               next = setup_object(s, page, next);
> +                               set_freepointer(s, p, next);
> +                       } else
>                                 set_freepointer(s, p, NULL);
>                 }
> -               page->freelist = fixup_red_left(s, start);
> +               start = fixup_red_left(s, start);
> +               start = setup_object(s, page, start);
> +               page->freelist = start;
>         }

Just want to double-check that this is correct.
We now do an additional setup_object call after the loop, but we do 1
less in the loop. So total number of calls should be the same, right?
However, after the loop we call setup_object for the first object (?),
but inside of the loop we skip the call for the last object (?). Am I
missing something, or we call ctor twice for the last object and don't
call it for the first one?


>         page->inuse = page->objects;
> --
> 2.19.0.397.gdd90340f6a-goog
Andrey Konovalov Sept. 21, 2018, 12:24 p.m. UTC | #2
On Fri, Sep 21, 2018 at 1:25 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Sep 19, 2018 at 8:54 PM, Andrey Konovalov <andreyknvl@google.com> wrote:

>>         if (!shuffle) {
>>                 for_each_object_idx(p, idx, s, start, page->objects) {
>> -                       setup_object(s, page, p);
>> -                       if (likely(idx < page->objects))
>> -                               set_freepointer(s, p, p + s->size);
>> -                       else
>> +                       if (likely(idx < page->objects)) {
>> +                               next = p + s->size;
>> +                               next = setup_object(s, page, next);
>> +                               set_freepointer(s, p, next);
>> +                       } else
>>                                 set_freepointer(s, p, NULL);
>>                 }
>> -               page->freelist = fixup_red_left(s, start);
>> +               start = fixup_red_left(s, start);
>> +               start = setup_object(s, page, start);
>> +               page->freelist = start;
>>         }
>
> Just want to double-check that this is correct.
> We now do an additional setup_object call after the loop, but we do 1
> less in the loop. So total number of calls should be the same, right?
> However, after the loop we call setup_object for the first object (?),
> but inside of the loop we skip the call for the last object (?). Am I
> missing something, or we call ctor twice for the last object and don't
> call it for the first one?

Inside the loop we call setup_object for the "next" object. So we
start iterating on the first one, but call setup_object for the
second. Then the loop moves on to the second one and calls
setup_object for the third. And so on. So the loop calls setup_object
for every object (including the last one) except for the first one.

The idea is that we want the freelist pointer that is stored in the
current object to have a tagged pointer to the next one, so we need to
assign a tag to the next object before storing the pointer in the
current one.
Dmitry Vyukov Sept. 24, 2018, 9:19 a.m. UTC | #3
On Fri, Sep 21, 2018 at 2:24 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> On Fri, Sep 21, 2018 at 1:25 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Wed, Sep 19, 2018 at 8:54 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
>
>>>         if (!shuffle) {
>>>                 for_each_object_idx(p, idx, s, start, page->objects) {
>>> -                       setup_object(s, page, p);
>>> -                       if (likely(idx < page->objects))
>>> -                               set_freepointer(s, p, p + s->size);
>>> -                       else
>>> +                       if (likely(idx < page->objects)) {
>>> +                               next = p + s->size;
>>> +                               next = setup_object(s, page, next);
>>> +                               set_freepointer(s, p, next);
>>> +                       } else
>>>                                 set_freepointer(s, p, NULL);
>>>                 }
>>> -               page->freelist = fixup_red_left(s, start);
>>> +               start = fixup_red_left(s, start);
>>> +               start = setup_object(s, page, start);
>>> +               page->freelist = start;
>>>         }
>>
>> Just want to double-check that this is correct.
>> We now do an additional setup_object call after the loop, but we do 1
>> less in the loop. So total number of calls should be the same, right?
>> However, after the loop we call setup_object for the first object (?),
>> but inside of the loop we skip the call for the last object (?). Am I
>> missing something, or we call ctor twice for the last object and don't
>> call it for the first one?
>
> Inside the loop we call setup_object for the "next" object. So we
> start iterating on the first one, but call setup_object for the
> second. Then the loop moves on to the second one and calls
> setup_object for the third. And so on. So the loop calls setup_object
> for every object (including the last one) except for the first one.
>
> The idea is that we want the freelist pointer that is stored in the
> current object to have a tagged pointer to the next one, so we need to
> assign a tag to the next object before storing the pointer in the
> current one.

Ah, OK, then false alarm.
diff mbox series

Patch

diff --git a/mm/slab.c b/mm/slab.c
index 6fdca9ec2ea4..fe0ddf08aa2c 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2574,7 +2574,7 @@  static void cache_init_objs(struct kmem_cache *cachep,
 
 	for (i = 0; i < cachep->num; i++) {
 		objp = index_to_obj(cachep, page, i);
-		kasan_init_slab_obj(cachep, objp);
+		objp = kasan_init_slab_obj(cachep, objp);
 
 		/* constructor could break poison info */
 		if (DEBUG == 0 && cachep->ctor) {
diff --git a/mm/slub.c b/mm/slub.c
index c4d5f4442ff1..75fc76e42a1e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1413,16 +1413,17 @@  static inline bool slab_free_freelist_hook(struct kmem_cache *s,
 #endif
 }
 
-static void setup_object(struct kmem_cache *s, struct page *page,
+static void *setup_object(struct kmem_cache *s, struct page *page,
 				void *object)
 {
 	setup_object_debug(s, page, object);
-	kasan_init_slab_obj(s, object);
+	object = kasan_init_slab_obj(s, object);
 	if (unlikely(s->ctor)) {
 		kasan_unpoison_object_data(s, object);
 		s->ctor(object);
 		kasan_poison_object_data(s, object);
 	}
+	return object;
 }
 
 /*
@@ -1530,16 +1531,16 @@  static bool shuffle_freelist(struct kmem_cache *s, struct page *page)
 	/* First entry is used as the base of the freelist */
 	cur = next_freelist_entry(s, page, &pos, start, page_limit,
 				freelist_count);
+	cur = setup_object(s, page, cur);
 	page->freelist = cur;
 
 	for (idx = 1; idx < page->objects; idx++) {
-		setup_object(s, page, cur);
 		next = next_freelist_entry(s, page, &pos, start, page_limit,
 			freelist_count);
+		next = setup_object(s, page, next);
 		set_freepointer(s, cur, next);
 		cur = next;
 	}
-	setup_object(s, page, cur);
 	set_freepointer(s, cur, NULL);
 
 	return true;
@@ -1561,7 +1562,7 @@  static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 	struct page *page;
 	struct kmem_cache_order_objects oo = s->oo;
 	gfp_t alloc_gfp;
-	void *start, *p;
+	void *start, *p, *next;
 	int idx, order;
 	bool shuffle;
 
@@ -1613,13 +1614,16 @@  static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 
 	if (!shuffle) {
 		for_each_object_idx(p, idx, s, start, page->objects) {
-			setup_object(s, page, p);
-			if (likely(idx < page->objects))
-				set_freepointer(s, p, p + s->size);
-			else
+			if (likely(idx < page->objects)) {
+				next = p + s->size;
+				next = setup_object(s, page, next);
+				set_freepointer(s, p, next);
+			} else
 				set_freepointer(s, p, NULL);
 		}
-		page->freelist = fixup_red_left(s, start);
+		start = fixup_red_left(s, start);
+		start = setup_object(s, page, start);
+		page->freelist = start;
 	}
 
 	page->inuse = page->objects;