From: Avihai Horon <avihaih@nvidia.com>
To: quintela@redhat.com
Cc: qemu-devel@nongnu.org, "Michael S . Tsirkin" <mst@redhat.com>,
Cornelia Huck <cohuck@redhat.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Alex Williamson <alex.williamson@redhat.com>,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
Yishai Hadas <yishaih@nvidia.com>,
Jason Gunthorpe <jgg@nvidia.com>, Mark Bloch <mbloch@nvidia.com>,
Maor Gottlieb <maorg@nvidia.com>,
Kirti Wankhede <kwankhede@nvidia.com>,
Tarun Gupta <targupta@nvidia.com>
Subject: Re: [PATCH 3/9] vfio/migration: Fix NULL pointer dereference bug
Date: Tue, 17 May 2022 15:28:33 +0300 [thread overview]
Message-ID: <ca7310a6-80fa-ec4d-f480-fce5ffb0c8cd@nvidia.com> (raw)
In-Reply-To: <87lev1sp8j.fsf@secure.mitica>
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),
next prev parent reply other threads:[~2022-05-17 14:02 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-12 15:43 [PATCH 0/9] vfio/migration: Implement VFIO migration protocol v2 Avihai Horon
2022-05-12 15:43 ` [PATCH 1/9] linux-headers: Update headers to v5.18-rc6 Avihai Horon
2022-05-12 15:43 ` [PATCH 2/9] vfio: Fix compilation errors caused by VFIO migration v1 deprecation Avihai Horon
2022-05-12 17:57 ` Alex Williamson
2022-05-12 18:25 ` Jason Gunthorpe
2022-05-12 21:11 ` Alex Williamson
2022-05-12 23:32 ` Jason Gunthorpe
2022-05-13 7:08 ` Cornelia Huck
2022-05-12 15:43 ` [PATCH 3/9] vfio/migration: Fix NULL pointer dereference bug Avihai Horon
2022-05-16 11:15 ` Juan Quintela
2022-05-17 12:28 ` Avihai Horon [this message]
2022-05-18 11:36 ` Juan Quintela
2022-05-12 15:43 ` [PATCH 4/9] vfio/migration: Skip pre-copy if dirty page tracking is not supported Avihai Horon
2022-05-16 11:22 ` Juan Quintela
2022-05-16 20:22 ` Alex Williamson
2022-05-16 23:08 ` Jason Gunthorpe
2022-05-17 16:00 ` Alex Williamson
2022-05-17 16:08 ` Jason Gunthorpe
2022-05-17 17:22 ` Alex Williamson
2022-05-17 17:39 ` Jason Gunthorpe
2022-05-18 3:46 ` Alex Williamson
2022-05-18 17:01 ` Jason Gunthorpe
2022-05-18 11:39 ` Juan Quintela
2022-05-18 15:50 ` Jason Gunthorpe
2022-05-24 15:11 ` Avihai Horon
2022-05-20 10:58 ` Joao Martins
2022-05-23 6:11 ` Avihai Horon
2022-05-23 9:45 ` Joao Martins
2022-05-12 15:43 ` [PATCH 5/9] migration/qemu-file: Add qemu_file_get_to_fd() Avihai Horon
2022-05-16 11:31 ` Juan Quintela
2022-05-17 12:36 ` Avihai Horon
2022-05-18 11:54 ` Juan Quintela
2022-05-18 15:42 ` Jason Gunthorpe
2022-05-18 16:00 ` Daniel P. Berrangé
2022-05-18 16:16 ` Jason Gunthorpe
2022-05-12 15:43 ` [PATCH 6/9] vfio/migration: Implement VFIO migration protocol v2 Avihai Horon
2022-05-23 18:14 ` Joao Martins
2022-05-24 15:35 ` Avihai Horon
2022-05-12 15:43 ` [PATCH 7/9] vfio/migration: Reset device if setting recover state fails Avihai Horon
2022-05-12 15:43 ` [PATCH 8/9] vfio: Alphabetize migration section of VFIO trace-events file Avihai Horon
2022-05-12 15:43 ` [PATCH 9/9] docs/devel: Align vfio-migration docs to VFIO migration v2 Avihai Horon
2022-05-12 18:02 ` [PATCH 0/9] vfio/migration: Implement VFIO migration protocol v2 Alex Williamson
2022-05-13 7:04 ` Cornelia Huck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ca7310a6-80fa-ec4d-f480-fce5ffb0c8cd@nvidia.com \
--to=avihaih@nvidia.com \
--cc=alex.williamson@redhat.com \
--cc=cohuck@redhat.com \
--cc=dgilbert@redhat.com \
--cc=jgg@nvidia.com \
--cc=kwankhede@nvidia.com \
--cc=maorg@nvidia.com \
--cc=mbloch@nvidia.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
--cc=targupta@nvidia.com \
--cc=yishaih@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).