| Message ID | 20190213020550.82453-1-cai@lca.pw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | slub: untag object before slab end | expand |
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>
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) >
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
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 > >
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) {
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(-)