On 5/16/2022 2:15 PM, Juan Quintela wrote:
External email: Use caution opening links or attachments Avihai Horon <avih...@nvidia.com> wrote:As part of its error flow, vfio_vmstate_change() accesses MigrationState->to_dst_file without any checks. This can cause a NULL pointer dereference if the error flow is taken and MigrationState->to_dst_file is not set. For example, this can happen if VM is started or stopped not during migration and vfio_vmstate_change() error flow is taken, as MigrationState->to_dst_file is not set at that time. Fix it by checking that MigrationState->to_dst_file is set before using it. Fixes: 02a7e71b1e5b ("vfio: Add VM state change handler to know state of VM") Signed-off-by: Avihai Horon <avih...@nvidia.com> --- hw/vfio/migration.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 835608cd23..21e8f9d4d4 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -744,7 +744,9 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state) */ error_report("%s: Failed to set device state 0x%x", vbasedev->name, (migration->device_state & mask) | value); - qemu_file_set_error(migrate_get_current()->to_dst_file, ret); + if (migrate_get_current()->to_dst_file) { + qemu_file_set_error(migrate_get_current()->to_dst_file, ret); + } }Reviewed-by: Juan Quintela <quint...@redhat.com> I mean that the change is right. But I am not so sure about using qemu_file_* operations outside of the migration_thread. I *think* that everything else that uses qemu_file_* functions is operating inside the migration_thread, and this one don't look like it. Furthermore, a fast look at qemu source, I can't see anyplace where we setup an error in a vmstate_change.
vfio_vmstate_change() will operate inside migration_thread if migration_thread is the one that called vm start/stop.
In cases where vm start/stop was not called by migration_thread, it will operate outside of migration_thread. But I think in such cases to_dst_file should not be set.
Ideally we should have returned error, but vm_state_notify() doesn't report errors.
Maybe later we can change vm_state_notify() to support error reporting, or instead of using to_dst_file to indicate an error, update some flag in VFIOMigration.
Thanks.
Later, Juan.vbasedev->migration->vm_running = running; trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
