Re: [PATCH] drm/ttm: fix out-of-bounds read in ttm_put_pages()

[Koenig, Christian]

Am 27.03.19 um 17:32 schrieb Jann Horn:
> When ttm_put_pages() tries to figure out whether it's dealing with
> transparent hugepages, it just reads past the bounds of the pages array
> without a check; instead, when the end of the array is reached, treat that
> as "we don't have enough pages left for a hugepage, so this isn't a
> hugepage".
>
>
> This was discovered by booting a libvirt VM with QXL graphics, which causes
> a KASAN splat. When I add the following line at the start of
> ttm_page_pool_get_pages():
>
>      pr_warn("ttm_put_pages(0x%lx, 0x%x)\n", (unsigned long)pages, npages);
>
> I get this dmesg output:
>
> [TTM] ttm_put_pages(0xffff8881e68c7ac0, 0x1)
> [...]
> ==================================================================
> [...]
> BUG: KASAN: slab-out-of-bounds in ttm_put_pages+0x5a0/0x680
> Read of size 8 at addr ffff8881e68c7ac8 by task kworker/3:2/189
> [...]
> CPU: 3 PID: 189 Comm: kworker/3:2 Not tainted 5.1.0-rc2+ #337
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> [...]
> Call Trace:
> [...]
>   kasan_report+0x14e/0x192
> [...]
>   ttm_put_pages+0x5a0/0x680
> [...]
>   ttm_pool_unpopulate_helper+0xcb/0xf0
>   ttm_tt_destroy.part.11+0x80/0x90
>   ttm_bo_cleanup_memtype_use+0x5f/0xe0
>   ttm_bo_cleanup_refs+0x262/0x2a0
>   ttm_bo_delayed_delete+0x2cb/0x2f0
> [...]
>   ttm_bo_delayed_workqueue+0x17/0x50
>   process_one_work+0x452/0x7d0
>   worker_thread+0x69/0x690
>   ? process_one_work+0x7d0/0x7d0
>   kthread+0x1ac/0x1d0
>   ? kthread_park+0xb0/0xb0
>   ret_from_fork+0x1f/0x30
>
> Allocated by task 189:
>   __kasan_kmalloc.constprop.9+0xa0/0xd0
>   __kmalloc_node+0x132/0x320
>   ttm_tt_init+0xc2/0x120
>   qxl_ttm_tt_create+0x77/0xa0
>   ttm_tt_create+0x90/0xf0
>   ttm_bo_handle_move_mem+0xc14/0xca0
>   ttm_bo_evict+0x240/0x540
>   ttm_mem_evict_first+0x240/0x2f0
>   ttm_bo_mem_space+0x492/0x660
>   ttm_bo_validate+0x18b/0x220
>   ttm_bo_init_reserved+0x603/0x6a0
>   ttm_bo_init+0xc7/0x1b0
>   qxl_bo_create+0x185/0x200
>   qxl_alloc_bo_reserved+0x7e/0x100
>   qxl_image_alloc_objects+0xdf/0x1a0
>   qxl_draw_dirty_fb+0x2ec/0x9bc
>   qxl_framebuffer_surface_dirty+0x15c/0x1f0
>   drm_fb_helper_dirty_work+0x25e/0x2a0
>   process_one_work+0x452/0x7d0
>   worker_thread+0x69/0x690
>   kthread+0x1ac/0x1d0
>   ret_from_fork+0x1f/0x30
> [...]
> The buggy address belongs to the object at ffff8881e68c7ac0
>                  which belongs to the cache kmalloc-8 of size 8
> The buggy address is located 0 bytes to the right of
>                  8-byte region [ffff8881e68c7ac0, ffff8881e68c7ac8)
> [...]
>
> Fixes: 5c42c64f7d54 ("drm/ttm: fix the fix for huge compound pages")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> ---
> I've tested that this makes the KASAN splat go away for me.
> I'm not very familiar with the graphics subsystem though,
> so it might well be that I've completely misdiagnosed what
> the root cause is.
>
>   drivers/gpu/drm/ttm/ttm_page_alloc.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index f841accc2c00..68912664a6df 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -731,9 +731,12 @@ static void ttm_put_pages(struct page **pages, unsigned npages, int flags,
>   
>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>   			if (!(flags & TTM_PAGE_FLAG_DMA32)) {
> -				for (j = 0; j < HPAGE_PMD_NR; ++j)
> +				for (j = 0; j < HPAGE_PMD_NR; ++j) {
> +					if (i + j >= npages)
> +						break;
>   					if (p++ != pages[i + j])
> -					    break;
> +						break;
> +				}

The code should probably be refactored to not do the loop in the first 
place.

But thanks for pointing out the problem, I'm going to take a look when I 
have time.

Christian.

>   
>   				if (j == HPAGE_PMD_NR)
>   					order = HPAGE_PMD_ORDER;
> @@ -766,9 +769,12 @@ static void ttm_put_pages(struct page **pages, unsigned npages, int flags,
>   			if (!p)
>   				break;
>   
> -			for (j = 0; j < HPAGE_PMD_NR; ++j)
> +			for (j = 0; j < HPAGE_PMD_NR; ++j) {
> +				if (i + j >= npages)
> +					break;
>   				if (p++ != pages[i + j])
>   				    break;
> +			}
>   
>   			if (j != HPAGE_PMD_NR)
>   				break;