Re: [PATCH 3/9] vfio/migration: Fix NULL pointer dereference bug

[Avihai Horon]

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),