From 02174878b1f7a70d3ac09c50c12799df0a1f9406 Mon Sep 17 00:00:00 2001 From: John Levon Date: Wed, 24 Nov 2021 23:59:24 +0000 Subject: verify region is mapped before acquiring dirty bitmap (#627) DMA regions not mapped by the server are not dirty tracked (the client must track changes via handling VFIO_USER_DMA_WRITE), but we weren't correctly enforcing this, which could segfault when ->dirty_bitmap was NULL. Found via AFL++. Signed-off-by: John Levon Reviewed-by: Thanos Makatos --- lib/dma.c | 12 ++++++++++++ test/py/test_dirty_pages.py | 19 +++++++++++++++++++ test/py/test_dma_unmap.py | 44 ++++++++++++++++++++++++++++++++++++++++++-- test/unit-tests.c | 45 --------------------------------------------- 4 files changed, 73 insertions(+), 47 deletions(-) diff --git a/lib/dma.c b/lib/dma.c index ffc0b6b..eed4370 100644 --- a/lib/dma.c +++ b/lib/dma.c @@ -285,6 +285,8 @@ get_bitmap_size(size_t region_size, size_t pgsize) static int dirty_page_logging_start_on_region(dma_memory_region_t *region, size_t pgsize) { + assert(region->fd != -1); + ssize_t size = get_bitmap_size(region->info.iova.iov_len, pgsize); if (size < 0) { return size; @@ -511,6 +513,11 @@ dma_controller_dirty_page_logging_start(dma_controller_t *dma, size_t pgsize) for (i = 0; i < (size_t)dma->nregions; i++) { dma_memory_region_t *region = &dma->regions[i]; + + if (region->fd == -1) { + continue; + } + if (dirty_page_logging_start_on_region(region, pgsize) < 0) { int _errno = errno; size_t j; @@ -594,6 +601,11 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr, region = &dma->regions[sg.region]; + if (region->fd == -1) { + vfu_log(dma->vfu_ctx, LOG_ERR, "region %d is not mapped", sg.region); + return ERROR_INT(EINVAL); + } + /* * TODO race condition between resetting bitmap and user calling * vfu_map_sg/vfu_unmap_sg(). diff --git a/test/py/test_dirty_pages.py b/test/py/test_dirty_pages.py index 9baf6cd..6d94a5f 100644 --- a/test/py/test_dirty_pages.py +++ b/test/py/test_dirty_pages.py @@ -82,6 +82,12 @@ def test_dirty_pages_setup(): msg(ctx, sock, VFIO_USER_DMA_MAP, payload, fds=[f.fileno()]) + payload = vfio_user_dma_map(argsz=len(vfio_user_dma_map()), + flags=(VFIO_USER_F_DMA_REGION_READ | VFIO_USER_F_DMA_REGION_WRITE), + offset=0, addr=0x20000, size=0x10000) + + msg(ctx, sock, VFIO_USER_DMA_MAP, payload) + def test_dirty_pages_short_write(): payload = struct.pack("I", 8) @@ -214,6 +220,19 @@ def test_dirty_pages_get_short_reply(): assert dirty_pages.flags == VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP +def test_get_dirty_page_bitmap_unmapped(): + argsz = len(vfio_user_dirty_pages()) + len(vfio_user_bitmap_range()) + 8 + + dirty_pages = vfio_user_dirty_pages(argsz=argsz, + flags=VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP) + bitmap = vfio_user_bitmap(pgsize=0x1000, size=8) + br = vfio_user_bitmap_range(iova=0x20000, size=0x10000, bitmap=bitmap) + + payload = bytes(dirty_pages) + bytes(br) + + msg(ctx, sock, VFIO_USER_DIRTY_PAGES, payload, expect=errno.EINVAL) + + def test_dirty_pages_get_unmodified(): argsz = len(vfio_user_dirty_pages()) + len(vfio_user_bitmap_range()) + 8 diff --git a/test/py/test_dma_unmap.py b/test/py/test_dma_unmap.py index f1da4b0..a449990 100644 --- a/test/py/test_dma_unmap.py +++ b/test/py/test_dma_unmap.py @@ -29,6 +29,7 @@ # import errno +import tempfile from libvfio_user import * ctx = None @@ -40,7 +41,18 @@ def test_dma_unmap_setup(): ctx = prepare_ctx_for_dma() assert ctx is not None - payload = struct.pack("II", 0, 0) + f = tempfile.TemporaryFile() + f.truncate(0x2000) + + mmap_areas = [(0x1000, 0x1000)] + + ret = vfu_setup_region(ctx, index=VFU_PCI_DEV_MIGR_REGION_IDX, size=0x2000, + flags=VFU_REGION_FLAG_RW, mmap_areas=mmap_areas, + fd=f.fileno()) + assert ret == 0 + + ret = vfu_realize_ctx(ctx) + assert ret == 0 sock = connect_client(ctx) @@ -76,13 +88,41 @@ def test_dma_unmap_dirty_bad_argsz(): argsz = len(vfio_user_dma_unmap()) + len(vfio_user_bitmap()) unmap = vfio_user_dma_unmap(argsz=argsz, - flags=VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP, addr=0x10000, size=4096) + flags=VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP, addr=0x1000, size=4096) bitmap = vfio_user_bitmap(pgsize=4096, size=(UINT64_MAX - argsz) + 8) payload = bytes(unmap) + bytes(bitmap) msg(ctx, sock, VFIO_USER_DMA_UNMAP, payload, expect=errno.EINVAL) +def test_dma_unmap_dirty_not_tracking(): + + argsz = len(vfio_user_dma_unmap()) + len(vfio_user_bitmap()) + 8 + unmap = vfio_user_dma_unmap(argsz=argsz, + flags=VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP, addr=0x1000, size=4096) + bitmap = vfio_user_bitmap(pgsize=4096, size=8) + payload = bytes(unmap) + bytes(bitmap) + bytes(8) + + msg(ctx, sock, VFIO_USER_DMA_UNMAP, payload, expect=errno.EINVAL) + + +def test_dma_unmap_dirty_not_mapped(): + + vfu_setup_device_migration_callbacks(ctx, offset=0x1000) + payload = vfio_user_dirty_pages(argsz=len(vfio_user_dirty_pages()), + flags=VFIO_IOMMU_DIRTY_PAGES_FLAG_START) + + msg(ctx, sock, VFIO_USER_DIRTY_PAGES, payload) + + argsz = len(vfio_user_dma_unmap()) + len(vfio_user_bitmap()) + 8 + unmap = vfio_user_dma_unmap(argsz=argsz, + flags=VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP, addr=0x1000, size=4096) + bitmap = vfio_user_bitmap(pgsize=4096, size=8) + payload = bytes(unmap) + bytes(bitmap) + bytes(8) + + msg(ctx, sock, VFIO_USER_DMA_UNMAP, payload, expect=errno.EINVAL) + + def test_dma_unmap_invalid_flags(): payload = vfio_user_dma_unmap(argsz=len(vfio_user_dma_unmap()), diff --git a/test/unit-tests.c b/test/unit-tests.c index 9dc222e..cab3fed 100644 --- a/test/unit-tests.c +++ b/test/unit-tests.c @@ -248,50 +248,6 @@ test_handle_dma_unmap(void **state UNUSED) } static void -test_handle_dma_unmap_dirty(void **state UNUSED) -{ - uint64_t bitmap = 0xdeadbeef; - size_t size = sizeof(struct vfio_user_dma_unmap) + sizeof(struct vfio_user_bitmap); - struct vfio_user_dma_unmap *dma_unmap = alloca(size); - dma_unmap->argsz = size + sizeof(bitmap); - dma_unmap->addr = 0x0; - dma_unmap->size = 0x1000; - dma_unmap->flags = VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP; - dma_unmap->bitmap->pgsize = 0x1000; - dma_unmap->bitmap->size = sizeof(bitmap); - - vfu_ctx.dma->nregions = 1; - vfu_ctx.dma->regions[0].info.iova.iov_base = (void *)0x0; - vfu_ctx.dma->regions[0].info.iova.iov_len = 0x1000; - vfu_ctx.dma->regions[0].fd = -1; - - /* - * TODO Hack to avoid mocking dma_controller_dirty_page_get since we're - * moving testing to Python. - */ - vfu_ctx.dma->dirty_pgsize = 0x1000; - vfu_ctx.dma->regions[0].dirty_bitmap = (void *)&bitmap; - - vfu_ctx.dma_unregister = mock_dma_unregister; - - expect_value(mock_dma_unregister, vfu_ctx, &vfu_ctx); - expect_check(mock_dma_unregister, info, check_dma_info, - &vfu_ctx.dma->regions[0].info); - will_return(mock_dma_unregister, 0); - - ret = handle_dma_unmap(&vfu_ctx, - mkmsg(VFIO_USER_DMA_UNMAP, &dma_unmap, size), - dma_unmap); - - assert_int_equal(0, ret); - assert_int_equal(0, vfu_ctx.dma->nregions); - assert_int_equal(size + sizeof(bitmap), msg.out_size); - assert_int_equal(0xdeadbeef, *(uint64_t *)(msg.out_data + size)); - free(msg.out_data); -} - - -static void test_dma_controller_add_region_no_fd(void **state UNUSED) { vfu_dma_addr_t dma_addr = (void *)0xdeadbeef; @@ -717,7 +673,6 @@ main(void) cmocka_unit_test_setup(test_dma_map_without_fd, setup), cmocka_unit_test_setup(test_dma_map_return_value, setup), cmocka_unit_test_setup(test_handle_dma_unmap, setup), - cmocka_unit_test_setup(test_handle_dma_unmap_dirty, setup), cmocka_unit_test_setup(test_dma_controller_add_region_no_fd, setup), cmocka_unit_test_setup(test_dma_controller_remove_region_mapped, setup), cmocka_unit_test_setup(test_dma_controller_remove_region_unmapped, setup), -- cgit v1.1