From: Andrey Drobyshev <[email protected]> In its current form, the mechanism for saving CPR FDs into hash table has flaws.
The CPR fd hash table uses the same CprFd for both key and value, and is created with a key destroy function. Inserting a second element with the same (name, id) key leads to a use after free. According to GLib docs [1], g_hash_table_insert() keeps the old key, replaces the value with the new element, and destroys the new key. Thus when inserting 2 CprFd elements fd1 and fd2 with equal keys we get: g_hash_table_insert(table, fd1, fd1); /* table has fd1 -> fd1 */ g_hash_table_insert(table, fd2, fd2); /* table has fd1 -> fd2, fd2 freed */ Any subsequent cpr_find_fd() for that key then returns freed memory. This is reachable whenever two CPR-aware objects register fds under the same name, and it happens long before an actual CPR, since cpr_save_fd() is typically called from .realize(), i.e. at boot time. Apart from the UAF itself, when the user decides to perform CPR migration, on CPR target we'd get corrupted FD from the hash table and fail somewhere in .realize() for affected devices. As a solution, let's altogether prohibit adding CprFds with duplicate keys. Since FDs are being registered for a subsequent CPR right at the startup in .realize(), that'll allow us to error out and exit QEMU (or fail a device hotplug op) gracefully instead of delaying potential problems for later. To do so, let's make cpr_save_fd() return 'bool' instead of 'void' and provide an 'errp' argument to it. That way caller becomes responsible for checking the return value and propagating the error. Also update all the callers to use the adjusted interface to keep things compilable and bisectable. [1] https://docs.gtk.org/glib/type_func.HashTable.insert.html Fixes: a181df93bb32 ("migration/cpr: use hashtable for cpr fds") Reviewed-by: Peter Xu <[email protected]> Reviewed-by: Cédric Le Goater <[email protected]> Signed-off-by: Andrey Drobyshev <[email protected]> Reviewed-by: Michael S. Tsirkin <[email protected]> Reviewed-by: Akihiko Odaki <[email protected]> Signed-off-by: Michael S. Tsirkin <[email protected]> Message-ID: <[email protected]> --- include/hw/vfio/vfio-cpr.h | 7 +++-- include/migration/cpr.h | 2 +- backends/hostmem-memfd.c | 5 ++- backends/hostmem-shm.c | 5 ++- backends/iommufd.c | 4 ++- hw/vfio/container-legacy.c | 7 +---- hw/vfio/cpr-legacy.c | 31 +++++++++++++----- hw/vfio/cpr.c | 6 ++-- hw/vfio/pci.c | 5 ++- migration/cpr.c | 64 ++++++++++++++++++++++++-------------- system/physmem.c | 5 +-- 11 files changed, 92 insertions(+), 49 deletions(-) diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h index ecabe0c747..9157a4ac64 100644 --- a/include/hw/vfio/vfio-cpr.h +++ b/include/hw/vfio/vfio-cpr.h @@ -60,7 +60,8 @@ void vfio_iommufd_cpr_register_device(struct VFIODevice *vbasedev); void vfio_iommufd_cpr_unregister_device(struct VFIODevice *vbasedev); void vfio_cpr_load_device(struct VFIODevice *vbasedev); -int vfio_cpr_group_get_device_fd(int d, const char *name); +int vfio_cpr_group_get_device_fd(struct VFIOGroup *group, const char *name, + Error **errp); bool vfio_cpr_container_match(struct VFIOLegacyContainer *container, struct VFIOGroup *group, int fd); @@ -71,8 +72,8 @@ void vfio_cpr_giommu_remap(struct VFIOContainer *bcontainer, bool vfio_cpr_ram_discard_replay_populated( struct VFIOContainer *bcontainer, const MemoryRegionSection *section); -void vfio_cpr_save_vector_fd(struct VFIOPCIDevice *vdev, const char *name, - int nr, int fd); +bool vfio_cpr_save_vector_fd(struct VFIOPCIDevice *vdev, const char *name, + int nr, int fd, Error **errp); int vfio_cpr_load_vector_fd(struct VFIOPCIDevice *vdev, const char *name, int nr); void vfio_cpr_delete_vector_fd(struct VFIOPCIDevice *vdev, const char *name, diff --git a/include/migration/cpr.h b/include/migration/cpr.h index 56fb67e6b4..3ad084d5ba 100644 --- a/include/migration/cpr.h +++ b/include/migration/cpr.h @@ -28,7 +28,7 @@ typedef struct CprState { extern CprState cpr_state; -void cpr_save_fd(const char *name, int id, int fd); +bool cpr_save_fd(const char *name, int id, int fd, Error **errp); void cpr_delete_fd(const char *name, int id); int cpr_find_fd(const char *name, int id); void cpr_resave_fd(const char *name, int id, int fd); diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c index 666593ff6c..67f1924a36 100644 --- a/backends/hostmem-memfd.c +++ b/backends/hostmem-memfd.c @@ -84,7 +84,10 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) if (fd < 0) { return false; } - cpr_save_fd(name, 0, fd); + if (!cpr_save_fd(name, 0, fd, errp)) { + close(fd); + return false; + } have_fd: backend->aligned = true; diff --git a/backends/hostmem-shm.c b/backends/hostmem-shm.c index 4766db6aad..adf60957c3 100644 --- a/backends/hostmem-shm.c +++ b/backends/hostmem-shm.c @@ -48,7 +48,10 @@ shm_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) if (fd < 0) { return false; } - cpr_save_fd(backend_name, 0, fd); + if (!cpr_save_fd(backend_name, 0, fd, errp)) { + close(fd); + return false; + } have_fd: /* Let's do the same as memory-backend-ram,share=on would do. */ diff --git a/backends/iommufd.c b/backends/iommufd.c index 15f2a51350..45fd2e44c7 100644 --- a/backends/iommufd.c +++ b/backends/iommufd.c @@ -85,7 +85,9 @@ static void iommufd_backend_complete(UserCreatable *uc, Error **errp) if (cpr_is_incoming()) { be->fd = cpr_find_fd(name, 0); } else { - cpr_save_fd(name, 0, be->fd); + if (!cpr_save_fd(name, 0, be->fd, errp)) { + return; + } } } else if (!g_file_test("/dev/iommu", G_FILE_TEST_EXISTS)) { error_setg(errp, "/dev/iommu does not exist" diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c index 7ac7b37214..50ca5a3a7e 100644 --- a/hw/vfio/container-legacy.c +++ b/hw/vfio/container-legacy.c @@ -847,13 +847,8 @@ static bool vfio_device_get(VFIOGroup *group, const char *name, g_autofree struct vfio_device_info *info = NULL; int fd; - fd = vfio_cpr_group_get_device_fd(group->fd, name); + fd = vfio_cpr_group_get_device_fd(group, name, errp); if (fd < 0) { - error_setg_errno(errp, errno, "error getting device from group %d", - group->groupid); - error_append_hint(errp, - "Verify all devices in group %d are bound to vfio-<bus> " - "or pci-stub and not already in use\n", group->groupid); return false; } diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c index 2d40d8baea..807ffacdbc 100644 --- a/hw/vfio/cpr-legacy.c +++ b/hw/vfio/cpr-legacy.c @@ -254,17 +254,33 @@ bool vfio_cpr_ram_discard_replay_populated(VFIOContainer *bcontainer, &vrdl->listener) == 0; } -int vfio_cpr_group_get_device_fd(int d, const char *name) +int vfio_cpr_group_get_device_fd(VFIOGroup *group, const char *name, + Error **errp) { + ERRP_GUARD(); const int id = 0; int fd = cpr_find_fd(name, id); - if (fd < 0) { - fd = ioctl(d, VFIO_GROUP_GET_DEVICE_FD, name); - if (fd >= 0) { - cpr_save_fd(name, id, fd); - } + if (fd >= 0) { + return fd; } + + fd = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name); + if (fd < 0) { + error_setg_errno(errp, errno, "error getting device from group %d", + group->groupid); + error_append_hint(errp, + "Verify all devices in group %d are bound to " + "vfio-<bus> or pci-stub and not already in use\n", + group->groupid); + return -1; + } + + if (!cpr_save_fd(name, id, fd, errp)) { + close(fd); + return -1; + } + return fd; } @@ -291,6 +307,7 @@ bool vfio_cpr_container_match(VFIOLegacyContainer *container, VFIOGroup *group, */ cpr_delete_fd("vfio_container_for_group", group->groupid); close(fd); - cpr_save_fd("vfio_container_for_group", group->groupid, container->fd); + cpr_save_fd("vfio_container_for_group", group->groupid, container->fd, + &error_abort); return true; } diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c index ffa4f8e099..6e1659dcdd 100644 --- a/hw/vfio/cpr.c +++ b/hw/vfio/cpr.c @@ -32,11 +32,11 @@ int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier, #define STRDUP_VECTOR_FD_NAME(vdev, name) \ g_strdup_printf("%s_%s", (vdev)->vbasedev.name, (name)) -void vfio_cpr_save_vector_fd(VFIOPCIDevice *vdev, const char *name, int nr, - int fd) +bool vfio_cpr_save_vector_fd(VFIOPCIDevice *vdev, const char *name, int nr, + int fd, Error **errp) { g_autofree char *fdname = STRDUP_VECTOR_FD_NAME(vdev, name); - cpr_save_fd(fdname, nr, fd); + return cpr_save_fd(fdname, nr, fd, errp); } int vfio_cpr_load_vector_fd(VFIOPCIDevice *vdev, const char *name, int nr) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 428ab2f069..ad13f2306f 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -76,7 +76,10 @@ static bool vfio_notifier_init(VFIOPCIDevice *vdev, EventNotifier *e, } fd = event_notifier_get_fd(e); - vfio_cpr_save_vector_fd(vdev, name, nr, fd); + if (!vfio_cpr_save_vector_fd(vdev, name, nr, fd, errp)) { + event_notifier_cleanup(e); + return false; + } return true; } diff --git a/migration/cpr.c b/migration/cpr.c index bca43e9bf3..cb027bba50 100644 --- a/migration/cpr.c +++ b/migration/cpr.c @@ -82,10 +82,29 @@ static GHashTable *get_cpr_fds_hash(void) return cpr_fds_hash; } +static CprFd *find_fd(const char *name, int id) +{ + CprFd key = { + .name = (char *)name, + .id = id, + }; + + return g_hash_table_lookup(get_cpr_fds_hash(), &key); +} + static void cpr_fd_hash_insert(CprFd *elem) { - /* Use the same CprFd as key and value. */ - g_hash_table_insert(get_cpr_fds_hash(), elem, elem); + GHashTable *hash = get_cpr_fds_hash(); + + /* + * We prohibit inserting duplicate keys into the hash table. All callers + * are responsible for checking that each added key is unique. + * + * 'elem' serves as both key and value, and a replacing insert would + * silently free it. Therefore we assert, not overwrite. + */ + g_assert(!g_hash_table_contains(hash, elem)); + g_hash_table_insert(hash, elem, elem); } static int cpr_fd_pre_save(void *opaque) @@ -127,26 +146,24 @@ static int cpr_fd_post_load(void *opaque, int version_id) return 0; } -void cpr_save_fd(const char *name, int id, int fd) +bool cpr_save_fd(const char *name, int id, int fd, Error **errp) { - CprFd *elem = g_new0(CprFd, 1); + CprFd *elem; + if (find_fd(name, id)) { + error_setg(errp, "cpr fd '%s' id %d is already registered", name, id); + return false; + } + + elem = g_new0(CprFd, 1); trace_cpr_save_fd(name, id, fd); elem->name = g_strdup(name); elem->namelen = strlen(name) + 1; elem->id = id; elem->fd = fd; cpr_fd_hash_insert(elem); -} -static CprFd *find_fd(const char *name, int id) -{ - CprFd key = { - .name = (char *)name, - .id = id, - }; - - return g_hash_table_lookup(get_cpr_fds_hash(), &key); + return true; } void cpr_delete_fd(const char *name, int id) @@ -173,14 +190,13 @@ int cpr_find_fd(const char *name, int id) void cpr_resave_fd(const char *name, int id, int fd) { CprFd *elem = find_fd(name, id); - int old_fd = elem ? elem->fd : -1; - if (old_fd < 0) { - cpr_save_fd(name, id, fd); - } else if (old_fd != fd) { + if (!elem) { + cpr_save_fd(name, id, fd, &error_abort); + } else if (elem->fd != fd) { error_report("internal error: cpr fd '%s' id %d value %d " "already saved with a different value %d", - name, id, fd, old_fd); + name, id, fd, elem->fd); g_assert_not_reached(); } } @@ -192,8 +208,9 @@ int cpr_open_fd(const char *path, int flags, const char *name, int id, if (fd < 0) { fd = qemu_open(path, flags, errp); - if (fd >= 0) { - cpr_save_fd(name, id, fd); + if (fd >= 0 && !cpr_save_fd(name, id, fd, errp)) { + close(fd); + fd = -1; } } return fd; @@ -400,10 +417,11 @@ int cpr_get_fd_param(const char *name, const char *fdname, int index, } } else { fd = monitor_fd_param(monitor_cur(), fdname, errp); - if (fd >= 0) { - cpr_save_fd(name, index, fd); - } else { + if (fd < 0) { error_prepend(errp, "Could not parse object fd %s:", fdname); + } else if (!cpr_save_fd(name, index, fd, errp)) { + close(fd); + fd = -1; } } return fd; diff --git a/system/physmem.c b/system/physmem.c index db3dda563d..a7300bb9d9 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -2456,8 +2456,9 @@ static int qemu_ram_get_shared_fd(const char *name, bool *reused, Error **errp) fd = qemu_shm_alloc(0, errp); } - if (fd >= 0) { - cpr_save_fd(name, 0, fd); + if (fd >= 0 && !cpr_save_fd(name, 0, fd, errp)) { + close(fd); + fd = -1; } *reused = false; return fd; -- MST
