| Message ID | 20220512154320.19697-4-avihaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/migration: Implement VFIO migration protocol v2 | expand |
Avihai Horon <avihaih@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 <avihaih@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 <quintela@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. Later, Juan. > vbasedev->migration->vm_running = running; > trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
On 5/16/2022 2:15 PM, Juan Quintela wrote: > External email: Use caution opening links or attachments > > > Avihai Horon <avihaih@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 <avihaih@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 <quintela@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),
Avihai Horon <avihaih@nvidia.com> wrote: > On 5/16/2022 2:15 PM, Juan Quintela wrote: >> External email: Use caution opening links or attachments >> >> >> Avihai Horon <avihaih@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 <avihaih@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 <quintela@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. I think that sounds like a better option. There are only two callers of vm_state_notify(): do_vm_stop() and vm_prepare_start() Both already support returning one error. Thanks, Juan.
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); + } } vbasedev->migration->vm_running = running; trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
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 <avihaih@nvidia.com> --- hw/vfio/migration.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)