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

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

[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@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
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;
+				}
 
 				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;
-- 
2.21.0.392.gf8f6787159e-goog

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@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> 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;