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);
+        }
     }
     vbasedev->migration->vm_running = running;
     trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
-- 
2.21.3

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.