From: Juan Quintela <quintela@redhat.com>
To: Avihai Horon <avihaih@nvidia.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: Wed, 18 May 2022 13:36:12 +0200 [thread overview]
Message-ID: <87mtfff4yb.fsf@secure.mitica> (raw)
In-Reply-To: <ca7310a6-80fa-ec4d-f480-fce5ffb0c8cd@nvidia.com> (Avihai Horon's message of "Tue, 17 May 2022 15:28:33 +0300")
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.
next prev parent reply other threads:[~2022-05-18 11:52 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
2022-05-18 11:36 ` Juan Quintela [this message]
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=87mtfff4yb.fsf@secure.mitica \
--to=quintela@redhat.com \
--cc=alex.williamson@redhat.com \
--cc=avihaih@nvidia.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=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).