On Tue, 3 Jan 2023 11:13:21 +0000
"Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> * 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>
> > Reviewed-by: Juan Quintela <quint...@redhat.com>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@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
> >
