On 9/28/26 5:26 PM, Peter Xu wrote:
> On Fri, Sep 18, 2026 at 04:46:52PM +0300, Andrey Drobyshev wrote:
>> 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: a181df93bb3 ("migration/cpr: use hashtable for cpr fds")
>> Reviewed-by: Cédric Le Goater <[email protected]>
>> Signed-off-by: Andrey Drobyshev <[email protected]>
>> ---
>> backends/hostmem-memfd.c | 5 ++++-
>> backends/hostmem-shm.c | 5 ++++-
>> backends/iommufd.c | 4 +++-
>> hw/vfio/container-legacy.c | 17 +++++++++-----
>> hw/vfio/cpr-legacy.c | 10 +++++----
>> hw/vfio/cpr.c | 6 ++---
>> hw/vfio/pci.c | 5 ++++-
>> include/hw/vfio/vfio-cpr.h | 6 ++---
>> include/migration/cpr.h | 2 +-
>> migration/cpr.c | 55
>> ++++++++++++++++++++++++++++++----------------
>> system/physmem.c | 5 +++--
>> 11 files changed, 78 insertions(+), 42 deletions(-)
>>
>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
>> index e21c5a3f2e2..a95a4368a9e 100644
>> --- a/backends/hostmem-memfd.c
>> +++ b/backends/hostmem-memfd.c
>> @@ -54,7 +54,10 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend,
>> Error **errp)
>> if (fd == -1) {
>> 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 806e2670e03..2fa7b823a62 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 15f2a513501..45fd2e44c7f 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 7ac7b372145..1d2c8356189 100644
>> --- a/hw/vfio/container-legacy.c
>> +++ b/hw/vfio/container-legacy.c
>> @@ -844,16 +844,21 @@ static void vfio_group_put(VFIOGroup *group)
>> static bool vfio_device_get(VFIOGroup *group, const char *name,
>> VFIODevice *vbasedev, Error **errp)
>> {
>> + ERRP_GUARD();
>> 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->fd, 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);
>> + if (!*errp) {
>
> IIUC this implies something not done right. If we add errp into
> vfio_cpr_group_get_device_fd(), then when fd<0 (failure case), we should
> set *errp always. Maybe this should be moved into it?
Agreed. I'll just move error handling into
vfio_cpr_group_get_device_fd(). Should look cleaner.
>> + 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 2d40d8baeaf..14a06a1fa59 100644
>> --- a/hw/vfio/cpr-legacy.c
>> +++ b/hw/vfio/cpr-legacy.c
>> @@ -254,15 +254,16 @@ 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(int d, const char *name, Error **errp)
>> {
>> 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 && !cpr_save_fd(name, id, fd, errp)) {
>
> IMHO it's slightly error prone to do oneliner like this, this might be
> relevant to the above !*errp question.
Ditto.
>> + close(fd);
>> + fd = -1;
>> }
>> }
>> return fd;
>> @@ -291,6 +292,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 ffa4f8e099d..6e1659dcddc 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 428ab2f0698..ad13f2306f1 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/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
>> index ecabe0c747d..e57766e7e2b 100644
>> --- a/include/hw/vfio/vfio-cpr.h
>> +++ b/include/hw/vfio/vfio-cpr.h
>> @@ -60,7 +60,7 @@ 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(int d, const char *name, Error **errp);
>>
>> bool vfio_cpr_container_match(struct VFIOLegacyContainer *container,
>> struct VFIOGroup *group, int fd);
>> @@ -71,8 +71,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 56fb67e6b4d..3ad084d5bab 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/migration/cpr.c b/migration/cpr.c
>> index bca43e9bf35..aeb9ad12392 100644
>> --- a/migration/cpr.c
>> +++ b/migration/cpr.c
>> @@ -82,10 +82,27 @@ 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();
>> +
>> + /*
>> + * The same CprFd is used as key and value, and the table owns the key.
>> + * Inserting a duplicate would destroy the new element while keeping it
>> + * as the value, so a duplicate key is a bug in the caller.
>
> IIUC this comment is slightly misleading. It almost says "the caller can't
> do this because the impl of the API does that", but logically the caller
> shouldn't care about the internal impl..
It's not the implementation we care about, but the documented behaviour.
And the docs say the key_destroy_func will be called on duplicated
keys, if provided. The "bug" on the caller's side here is that caller
doesn't take it into account, yet providing same object for both key and
value.
> There're two relevant issues, IMHO:
>
> (1) the memory crash described in this patch, solid, needs fixing one way
> or another
>
> (2) if there's real use case of injecting the same (name, id) fd twice, or
> should we forbid it?
>
> For (1), fixing it like this patch would work, but then the comment is
> wrong here, and it means we already said YES to (2), hence (1) relies on
> (2).
>
> But I do see the existance of cpr_resave_fd(). I confess I don't know why
> it existed and didn't check. That looks exactly like sometimes we can save
> the same fd twice, if the 1st one is fd=-1. Any quick answer?
AFAICT the sole purpose of cpr_resave_fd() is to be called from
different sites with the same (name, id, fd), to make sure the key is
registered in the table. What it does:
- no entry - save it into hash table;
- same FD already there - do nothing, table isn't updated;
- different FD present - abort as an internal error.
That means there's no case of injecting the same (name, id) twice into
the hash table. That is actually consistent with our patch here.
I'd leave the code as-is and only reword the comment, if that's fine
with you.
Thanks,
Andrey
