| Message ID | 20221229110345.12480-5-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> > Reviewed-by: Juan Quintela <quintela@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> It might be worth posting this patch separately since it's a simple fix and should go in sooner. Dave > --- > 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 e1413ac90c..09fe7c1de2 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -743,7 +743,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), > -- > 2.26.3 >
On 03/01/2023 13:13, Dr. David Alan Gilbert 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> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > It might be worth posting this patch separately since it's a simple fix > and should go in sooner. Sure, I will wait for Alex's feedback on the series and consider sending it separately. Thanks. > Dave > >> --- >> 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 e1413ac90c..09fe7c1de2 100644 >> --- a/hw/vfio/migration.c >> +++ b/hw/vfio/migration.c >> @@ -743,7 +743,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), >> -- >> 2.26.3 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
On Tue, 3 Jan 2023 11:13:21 +0000 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * 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> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > It might be worth posting this patch separately since it's a simple fix > and should go in sooner. There's no upstream implementation of a v1 migration driver, it's deprecated in the kernel, and only enabled in QEMU via an experimental option. It seems sufficient as done here to make it a separate fix for downstreams that may want to backport separately, but given the remaining v1 status, I don't think I'd bother hurrying it otherwise. Thanks, Alex > > --- > > 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 e1413ac90c..09fe7c1de2 100644 > > --- a/hw/vfio/migration.c > > +++ b/hw/vfio/migration.c > > @@ -743,7 +743,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), > > -- > > 2.26.3 > >
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index e1413ac90c..09fe7c1de2 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -743,7 +743,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> Reviewed-by: Juan Quintela <quintela@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- hw/vfio/migration.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)