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 <john.levon@nutanix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@nutanix.com>

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),