From: Peter Xu <peterx@redhat.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: qemu-devel@nongnu.org, "Markus Armbruster" <armbru@redhat.com>,
"Peter Maydell" <peter.maydell@linaro.org>,
"Eduardo Habkost" <eduardo@habkost.net>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>,
"Igor Mammedov" <imammedo@redhat.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
"Alex Williamson" <alex.williamson@redhat.com>,
"Dr . David Alan Gilbert" <dave@treblig.org>,
"Philippe Mathieu-Daudé" <philmd@linaro.org>,
"Cédric Le Goater" <clg@redhat.com>,
"Fabiano Rosas" <farosas@suse.de>,
"Juraj Marcin" <jmarcin@redhat.com>
Subject: Re: [PATCH RFC v2 0/7] QOM: Singleton interface
Date: Wed, 30 Oct 2024 13:51:54 -0400 [thread overview]
Message-ID: <ZyJyOnHidTsPAXrR@x1n> (raw)
In-Reply-To: <ZyJbRZ02wX4XM-iR@redhat.com>
On Wed, Oct 30, 2024 at 04:13:57PM +0000, Daniel P. Berrangé wrote:
> On Wed, Oct 30, 2024 at 09:13:13AM -0400, Peter Xu wrote:
> > On Wed, Oct 30, 2024 at 09:48:07AM +0000, Daniel P. Berrangé wrote:
> > > On Tue, Oct 29, 2024 at 05:16:00PM -0400, Peter Xu wrote:
> > > > v1: https://lore.kernel.org/r/20241024165627.1372621-1-peterx@redhat.com
> > >
> > > > Meanwhile, migration has a long standing issue on current_migration
> > > > pointer, where it can point to freed data after the migration object is
> > > > finalized. It is debatable that the pointer can be cleared after the main
> > > > thread (1) join() the migration thread first, then (2) release the last
> > > > refcount for the migration object and clear the pointer. However there's
> > > > still major challenges [1]. With singleton, we could have a slightly but
> > > > hopefully working workaround to clear the pointer during finalize().
> > >
> > > I'm still not entirely convinced that this singleton proposal is
> > > fixing the migration problem correctly.
> > >
> > > Based on discussions in v1, IIUC, the situation is that we have
> > > migration_shutdown() being called from qemu_cleanup(). The former
> > > will call object_unref(current_migration), but there may still
> > > be background migration threads running that access 'current_migration',
> > > and thus a potential use-after-free.
> >
> > migration thread is fine, it takes a refcount at the entry.
> >
> > And btw, taking it at the entry is racy, we've just fixed it, see (in my
> > next migration pull):
> >
> > https://lore.kernel.org/qemu-devel/20241024213056.1395400-2-peterx@redhat.com/
>
> Yep, acquiring the refcount immediately before thread-create
> is what I meant.
>
> > The access reported was, IIUC, outside migration code, but after both
> > main/migration threads released the refcount, hence after finalize(). It
> > could be a random migration_is_running() call very late in device code, for
> > example.
>
>
>
> >
> > >
> > > Based on what the 7th patch here does, the key difference is that
> > > the finalize() method for MigrationState will set 'current_migration'
> > > to NULL after free'ing it.
> >
> > Yes. But this show case series isn't complete. We need a migration-side
> > lock finally to make it safe to access. For that, see:
> >
> > https://lore.kernel.org/qemu-devel/20241024213056.1395400-9-peterx@redhat.com/
> >
> > >
> > > I don't believe that is safe.
> >
> > I hope after the other series applied it will be 100% safe, even though I
> > agree it's tricky. But hopefully QOM is very clean, the trickly part is
> > still within migration, and it should be less tricky than migration
> > implement a refcount on top of Object..
>
> Ok, so with the other series applied, this does look safe, but
> it also doesn't seem to really have any dependancy on the
> single interface code. Patch 7 here looks sufficient, in combo
> with the other 2 series to avoid the use-after-free flaws.
Patch 7, when applied without patch 6 and prior, will crash in
device-introspect-test, trying to create yet another migration object when
processing the "device-list-properties" QMP command. And it turns out
that's also not the only way QEMU can crash by that.
Fundamentally it's because patch 7 has global operations within
init()/finalize() to fix the migration dangling pointer, hence it must not
be instanciated more than once.
It's also probably because I always think singleton can be useful in
general to QEMU's device model where can be special devices all over the
places that I'm not aware of. I didn't work on a lot of QEMU devices, but
with that limited experience I still stumbled upon two devices (if taking
migration object as one..) that might benefit from it.
That leads to this whole series, which is also the cleanest so far I can
think of to solve the immediate migration UAF.
Thanks,
>
> > I think the question is whether things like migration_is_running() is
> > allowed to be used anywhere, even after migration_shutdown(). My answer
> > is, it should be ok to be used anywhere, and we don't necessarilly need to
> > limit that. In that case the caller doesn't need to take a refcount
> > because it's an immediate query. It can simply check its existance with
> > the lock (after my patch 8 of the other series applied, which depends on
> > this qom series).
>
> Agree, and from a practical POV, I think it would be impossible to
> require a ref count be held from other non-migration threads, so the
> locking around 'current_migration' looks like the only practical
> option.
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
--
Peter Xu
next prev parent reply other threads:[~2024-10-30 17:52 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-29 21:16 [PATCH RFC v2 0/7] QOM: Singleton interface Peter Xu
2024-10-29 21:16 ` [PATCH RFC v2 1/7] qom: Track dynamic initiations of random object class Peter Xu
2024-10-29 21:16 ` [PATCH RFC v2 2/7] qom: TYPE_SINGLETON interface Peter Xu
2024-10-29 21:16 ` [PATCH RFC v2 3/7] qdev: Make device_set_realized() be fully prepared with !machine Peter Xu
2024-10-29 21:16 ` [PATCH RFC v2 4/7] qdev: Make qdev_get_machine() safe before machine creates Peter Xu
2024-10-29 21:16 ` [PATCH RFC v2 5/7] x86/iommu: Make x86-iommu a singleton object Peter Xu
2024-10-30 10:33 ` Daniel P. Berrangé
2024-10-30 13:01 ` Peter Xu
2024-10-30 13:07 ` Daniel P. Berrangé
2024-10-30 14:33 ` Peter Xu
2024-10-29 21:16 ` [PATCH RFC v2 6/7] migration: Make migration object " Peter Xu
2024-10-29 21:16 ` [PATCH RFC v2 7/7] migration: Reset current_migration properly Peter Xu
2024-10-30 9:48 ` [PATCH RFC v2 0/7] QOM: Singleton interface Daniel P. Berrangé
2024-10-30 13:13 ` Peter Xu
2024-10-30 16:13 ` Daniel P. Berrangé
2024-10-30 17:51 ` Peter Xu [this message]
2024-10-30 17:58 ` Daniel P. Berrangé
2024-10-30 18:55 ` Peter Xu
2024-10-30 18:07 ` Daniel P. Berrangé
2024-10-30 19:08 ` Peter Xu
2024-10-31 15:57 ` Daniel P. Berrangé
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=ZyJyOnHidTsPAXrR@x1n \
--to=peterx@redhat.com \
--cc=alex.williamson@redhat.com \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=clg@redhat.com \
--cc=dave@treblig.org \
--cc=eduardo@habkost.net \
--cc=farosas@suse.de \
--cc=imammedo@redhat.com \
--cc=jmarcin@redhat.com \
--cc=mark.cave-ayland@ilande.co.uk \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
/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).