slub: untag object before slab end
diff mbox series

Message ID 20190213020550.82453-1-cai@lca.pw
State New
Headers show
Series
  • slub: untag object before slab end
Related show

Commit Message

Qian Cai Feb. 13, 2019, 2:05 a.m. UTC
get_freepointer() could return NULL if there is no more free objects in
the slab. However, it could return a tagged pointer (like
0x2200000000000000) with KASAN_SW_TAGS which would escape the NULL
object checking in check_valid_pointer() and trigger errors below, so
untag the object before checking for a NULL object there.

[   35.797667] BUG kmalloc-256 (Not tainted): Freepointer corrupt
[   35.803584] -----------------------------------------------------------------------------
[   35.803584]
[   35.813368] Disabling lock debugging due to kernel taint
[   35.818766] INFO: Allocated in build_sched_domains+0x28c/0x495c age=92 cpu=0 pid=1
[   35.826443] 	__kmalloc_node+0x4ac/0x508
[   35.830343] 	build_sched_domains+0x28c/0x495c
[   35.834764] 	sched_init_domains+0x184/0x1d8
[   35.839012] 	sched_init_smp+0x38/0xd4
[   35.842732] 	kernel_init_freeable+0x67c/0x1104
[   35.847243] 	kernel_init+0x18/0x2a4
[   35.850790] 	ret_from_fork+0x10/0x18
[   35.854423] INFO: Freed in destroy_sched_domain+0xa0/0xcc age=11 cpu=0 pid=1
[   35.861569] 	destroy_sched_domain+0xa0/0xcc
[   35.865814] 	cpu_attach_domain+0x304/0xb34
[   35.869971] 	build_sched_domains+0x4654/0x495c
[   35.874480] 	sched_init_domains+0x184/0x1d8
[   35.878724] 	sched_init_smp+0x38/0xd4
[   35.882443] 	kernel_init_freeable+0x67c/0x1104
[   35.886953] 	kernel_init+0x18/0x2a4
[   35.890495] 	ret_from_fork+0x10/0x18
[   35.894128] INFO: Slab 0x(____ptrval____) objects=85 used=0 fp=0x(____ptrval____) flags=0x7ffffffc000200
[   35.903733] INFO: Object 0x(____ptrval____) @offset=38528 fp=0x(____ptrval____)
[   35.903733]
[   35.912637] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
[   35.922155] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
[   35.931672] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
[   35.941190] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
[   35.950707] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
[   35.960224] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
[   35.969741] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
[   35.979258] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
[   35.988776] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   35.998206] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.007636] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.017065] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.026494] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.035923] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.045353] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.054783] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.064212] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.073642] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.083071] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.092501] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.101930] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.111359] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.120788] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   36.130218] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5  kkkkkkkkkkkkkkk.
[   36.139647] Redzone (____ptrval____): bb bb bb bb bb bb bb bb                          ........
[   36.148462] Padding (____ptrval____): 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  ZZZZZZZZZZZZZZZZ
[   36.157979] Padding (____ptrval____): 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  ZZZZZZZZZZZZZZZZ
[   36.167496] Padding (____ptrval____): 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  ZZZZZZZZZZZZZZZZ
[   36.177021] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G    B             5.0.0-rc6+ #41
[   36.184854] Call trace:
[   36.187328]  dump_backtrace+0x0/0x450
[   36.191032]  show_stack+0x20/0x2c
[   36.194385]  __dump_stack+0x20/0x28
[   36.197911]  dump_stack+0xa0/0xfc
[   36.201265]  print_trailer+0x1a8/0x1bc
[   36.205057]  object_err+0x40/0x50
[   36.208408]  check_object+0x214/0x2b8
[   36.212111]  __free_slab+0x9c/0x31c
[   36.215638]  discard_slab+0x78/0xa8
[   36.219165]  kfree+0x918/0x980
[   36.222259]  destroy_sched_domain+0xa0/0xcc
[   36.226489]  cpu_attach_domain+0x304/0xb34
[   36.230631]  build_sched_domains+0x4654/0x495c
[   36.235125]  sched_init_domains+0x184/0x1d8
[   36.239357]  sched_init_smp+0x38/0xd4
[   36.243060]  kernel_init_freeable+0x67c/0x1104
[   36.247555]  kernel_init+0x18/0x2a4
[   36.251083]  ret_from_fork+0x10/0x18

Signed-off-by: Qian Cai <cai@lca.pw>
---

Depends on slub-fix-slab_consistency_checks-kasan_sw_tags.patch.

 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Pekka Enberg Feb. 13, 2019, 7:32 a.m. UTC | #1
On 13/02/2019 4.05, Qian Cai wrote:
> get_freepointer() could return NULL if there is no more free objects in
> the slab. However, it could return a tagged pointer (like
> 0x2200000000000000) with KASAN_SW_TAGS which would escape the NULL
> object checking in check_valid_pointer() and trigger errors below, so
> untag the object before checking for a NULL object there.

Reviewed-by: Pekka Enberg <penberg@kernel.org>
Andrey Konovalov Feb. 13, 2019, 10:31 a.m. UTC | #2
On Wed, Feb 13, 2019 at 3:06 AM Qian Cai <cai@lca.pw> wrote:
>
> get_freepointer() could return NULL if there is no more free objects in
> the slab. However, it could return a tagged pointer (like
> 0x2200000000000000) with KASAN_SW_TAGS which would escape the NULL
> object checking in check_valid_pointer() and trigger errors below, so
> untag the object before checking for a NULL object there.

I think this solution is just masking the issue. get_freepointer()
shouldn't return tagged NULLs. Apparently when we save a freelist
pointer, the object where the pointer gets written is tagged
differently, than this same object when the pointer gets read. I found
one case where this happens (the last patch out my 5 patch series),
but apparently there are more.

>
> [   35.797667] BUG kmalloc-256 (Not tainted): Freepointer corrupt
> [   35.803584] -----------------------------------------------------------------------------
> [   35.803584]
> [   35.813368] Disabling lock debugging due to kernel taint
> [   35.818766] INFO: Allocated in build_sched_domains+0x28c/0x495c age=92 cpu=0 pid=1
> [   35.826443]  __kmalloc_node+0x4ac/0x508
> [   35.830343]  build_sched_domains+0x28c/0x495c
> [   35.834764]  sched_init_domains+0x184/0x1d8
> [   35.839012]  sched_init_smp+0x38/0xd4
> [   35.842732]  kernel_init_freeable+0x67c/0x1104
> [   35.847243]  kernel_init+0x18/0x2a4
> [   35.850790]  ret_from_fork+0x10/0x18
> [   35.854423] INFO: Freed in destroy_sched_domain+0xa0/0xcc age=11 cpu=0 pid=1
> [   35.861569]  destroy_sched_domain+0xa0/0xcc
> [   35.865814]  cpu_attach_domain+0x304/0xb34
> [   35.869971]  build_sched_domains+0x4654/0x495c
> [   35.874480]  sched_init_domains+0x184/0x1d8
> [   35.878724]  sched_init_smp+0x38/0xd4
> [   35.882443]  kernel_init_freeable+0x67c/0x1104
> [   35.886953]  kernel_init+0x18/0x2a4
> [   35.890495]  ret_from_fork+0x10/0x18
> [   35.894128] INFO: Slab 0x(____ptrval____) objects=85 used=0 fp=0x(____ptrval____) flags=0x7ffffffc000200
> [   35.903733] INFO: Object 0x(____ptrval____) @offset=38528 fp=0x(____ptrval____)
> [   35.903733]
> [   35.912637] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
> [   35.922155] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
> [   35.931672] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
> [   35.941190] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
> [   35.950707] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
> [   35.960224] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
> [   35.969741] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
> [   35.979258] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb  ................
> [   35.988776] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   35.998206] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.007636] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.017065] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.026494] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.035923] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.045353] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.054783] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.064212] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.073642] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.083071] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.092501] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.101930] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.111359] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.120788] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
> [   36.130218] Object (____ptrval____): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5  kkkkkkkkkkkkkkk.
> [   36.139647] Redzone (____ptrval____): bb bb bb bb bb bb bb bb                          ........
> [   36.148462] Padding (____ptrval____): 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  ZZZZZZZZZZZZZZZZ
> [   36.157979] Padding (____ptrval____): 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  ZZZZZZZZZZZZZZZZ
> [   36.167496] Padding (____ptrval____): 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  ZZZZZZZZZZZZZZZZ
> [   36.177021] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G    B             5.0.0-rc6+ #41
> [   36.184854] Call trace:
> [   36.187328]  dump_backtrace+0x0/0x450
> [   36.191032]  show_stack+0x20/0x2c
> [   36.194385]  __dump_stack+0x20/0x28
> [   36.197911]  dump_stack+0xa0/0xfc
> [   36.201265]  print_trailer+0x1a8/0x1bc
> [   36.205057]  object_err+0x40/0x50
> [   36.208408]  check_object+0x214/0x2b8
> [   36.212111]  __free_slab+0x9c/0x31c
> [   36.215638]  discard_slab+0x78/0xa8
> [   36.219165]  kfree+0x918/0x980
> [   36.222259]  destroy_sched_domain+0xa0/0xcc
> [   36.226489]  cpu_attach_domain+0x304/0xb34
> [   36.230631]  build_sched_domains+0x4654/0x495c
> [   36.235125]  sched_init_domains+0x184/0x1d8
> [   36.239357]  sched_init_smp+0x38/0xd4
> [   36.243060]  kernel_init_freeable+0x67c/0x1104
> [   36.247555]  kernel_init+0x18/0x2a4
> [   36.251083]  ret_from_fork+0x10/0x18
>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>
> Depends on slub-fix-slab_consistency_checks-kasan_sw_tags.patch.
>
>  mm/slub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 4a61959e1887..2fd1cf39914c 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -503,11 +503,11 @@ static inline int check_valid_pointer(struct kmem_cache *s,
>  {
>         void *base;
>
> +       object = kasan_reset_tag(object);
>         if (!object)
>                 return 1;
>
>         base = page_address(page);
> -       object = kasan_reset_tag(object);
>         object = restore_red_left(s, object);
>         if (object < base || object >= base + page->objects * s->size ||
>                 (object - base) % s->size) {
> --
> 2.17.2 (Apple Git-113)
>
Qian Cai Feb. 13, 2019, 9:12 p.m. UTC | #3
On Wed, 2019-02-13 at 11:31 +0100, Andrey Konovalov wrote:
> On Wed, Feb 13, 2019 at 3:06 AM Qian Cai <cai@lca.pw> wrote:
> > 
> > get_freepointer() could return NULL if there is no more free objects in
> > the slab. However, it could return a tagged pointer (like
> > 0x2200000000000000) with KASAN_SW_TAGS which would escape the NULL
> > object checking in check_valid_pointer() and trigger errors below, so
> > untag the object before checking for a NULL object there.
> 
> I think this solution is just masking the issue. get_freepointer()
> shouldn't return tagged NULLs. Apparently when we save a freelist
> pointer, the object where the pointer gets written is tagged
> differently, than this same object when the pointer gets read. I found
> one case where this happens (the last patch out my 5 patch series),
> but apparently there are more.

Well, the problem is that,

__free_slab
  for_each_object(p, s, page_address(page) [1]
    check_object(s, page, p ...)
      get_freepointer(s, p)

[1]: p += s->size

page_address() tags the address using page_kasan_tag(page), so each "p" here has
that tag.

However, at beginning in allocate_slab(), it tags each object with a random tag,
and then calls set_freepointer(s, p, NULL)

As the result, get_freepointer() returns a tagged NULL because it never be able
to obtain the original tag of the object anymore, and this calculation is now
wrong.

return (void *)((unsigned long)ptr ^ s->random ^ ptr_addr);

This also explain why this patch also works, as it unifies the tags.

https://marc.info/?l=linux-mm&m=154955366113951&w=2
Andrey Konovalov Feb. 14, 2019, 12:26 a.m. UTC | #4
On Wed, Feb 13, 2019 at 10:12 PM Qian Cai <cai@lca.pw> wrote:
>
> On Wed, 2019-02-13 at 11:31 +0100, Andrey Konovalov wrote:
> > On Wed, Feb 13, 2019 at 3:06 AM Qian Cai <cai@lca.pw> wrote:
> > >
> > > get_freepointer() could return NULL if there is no more free objects in
> > > the slab. However, it could return a tagged pointer (like
> > > 0x2200000000000000) with KASAN_SW_TAGS which would escape the NULL
> > > object checking in check_valid_pointer() and trigger errors below, so
> > > untag the object before checking for a NULL object there.
> >
> > I think this solution is just masking the issue. get_freepointer()
> > shouldn't return tagged NULLs. Apparently when we save a freelist
> > pointer, the object where the pointer gets written is tagged
> > differently, than this same object when the pointer gets read. I found
> > one case where this happens (the last patch out my 5 patch series),
> > but apparently there are more.
>
> Well, the problem is that,
>
> __free_slab
>   for_each_object(p, s, page_address(page) [1]
>     check_object(s, page, p ...)
>       get_freepointer(s, p)
>
> [1]: p += s->size
>
> page_address() tags the address using page_kasan_tag(page), so each "p" here has
> that tag.

Ah, I see what the issue is. With those 5 patches page_address()
should return 0xff-tagged pointer here, but when we set_freepointer()
the tag indeed might be different. OK, I think that patch that you
linked below is the better way to deal with this. I've added a
detailed comment to it and sent it.

Thanks!

>
> However, at beginning in allocate_slab(), it tags each object with a random tag,
> and then calls set_freepointer(s, p, NULL)
>
> As the result, get_freepointer() returns a tagged NULL because it never be able
> to obtain the original tag of the object anymore, and this calculation is now
> wrong.
>
> return (void *)((unsigned long)ptr ^ s->random ^ ptr_addr);
>
> This also explain why this patch also works, as it unifies the tags.
>
> https://marc.info/?l=linux-mm&m=154955366113951&w=2
>
>

Patch
diff mbox series

diff --git a/mm/slub.c b/mm/slub.c
index 4a61959e1887..2fd1cf39914c 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -503,11 +503,11 @@  static inline int check_valid_pointer(struct kmem_cache *s,
 {
 	void *base;
 
+	object = kasan_reset_tag(object);
 	if (!object)
 		return 1;
 
 	base = page_address(page);
-	object = kasan_reset_tag(object);
 	object = restore_red_left(s, object);
 	if (object < base || object >= base + page->objects * s->size ||
 		(object - base) % s->size) {