thr3ads.net - Virtualization - [PATCH V3] virtio: disable notification hardening by default [Jun 2022]

If this information is useful, please help other people find it:
Share via:

Michael S. Tsirkin

2022-Jun-29 06:31 UTC

[PATCH V3] virtio: disable notification hardening by default

On Wed, Jun 29, 2022 at 12:07:11PM +0800, Jason Wang
wrote:> On Tue, Jun 28, 2022 at 2:17 PM Jason Wang <jasowang at redhat.com>
wrote:
> >
> > On Tue, Jun 28, 2022 at 1:00 PM Michael S. Tsirkin <mst at
redhat.com> wrote:
> > >
> > > On Tue, Jun 28, 2022 at 11:49:12AM +0800, Jason Wang wrote:
> > > > > Heh. Yea sure. But things work fine for people. What is
the chance
> > > > > your review found and fixed all driver bugs?
> > > >
> > > > I don't/can't audit all bugs but the race between
open/close against
> > > > ready/reset. It looks to me a good chance to fix them all
but if you
> > > > think differently, let me know
> > > >
> > > > > After two attempts
> > > > > I don't feel like hoping audit will fix all bugs.
> > > >
> > > > I've started the auditing and have 15+ patches in the
queue. (only
> > > > covers bluetooth, console, pmem, virtio-net and caif).
Spotting the
> > > > issue is not hard but the testing, It would take at least
the time of
> > > > one release to finalize I guess.
> > >
> > > Absolutely. So I am looking for a way to implement hardening that
does
> > > not break existing drivers.
> >
> > I totally agree with you to seek a way without bothering the drivers.
> > Just wonder if this is possbile.
> >
> > >
> > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > The reason config was kind of easy is that
config interrupt is rarely
> > > > > > > vital for device function so arbitrarily
deferring that does not lead to
> > > > > > > deadlocks - what you are trying to do with VQ
interrupts is
> > > > > > > fundamentally different. Things are
especially bad if we just drop
> > > > > > > an interrupt but deferring can lead to
problems too.
> > > > > >
> > > > > > I'm not sure I see the difference,
disable_irq() stuffs also delay the
> > > > > > interrupt processing until enable_irq().
> > > > >
> > > > >
> > > > > Absolutely. I am not at all sure disable_irq fixes all
problems.
> > > > >
> > > > > > >
> > > > > > > Consider as an example
> > > > > > >     virtio-net: fix race between ndo_open()
and virtio_device_ready()
> > > > > > > if you just defer vq interrupts you get
deadlocks.
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > I don't see a deadlock here, maybe you can
show more detail on this?
> > > > >
> > > > > What I mean is this: if we revert the above commit,
things still
> > > > > work (out of spec, but still). If we revert and defer
interrupts until
> > > > > device ready then ndo_open that triggers before device
ready deadlocks.
> > > >
> > > > Ok, I guess you meant on a hypervisor that is strictly
written with spec.
> > >
> > > I mean on hypervisor that starts processing queues after getting
a kick
> > > even without DRIVER_OK.
> >
> > Oh right.
> >
> > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > So, thinking about all this, how about a
simple per vq flag meaning
> > > > > > > "this vq was kicked since reset"?
> > > > > >
> > > > > > And ignore the notification if vq is not kicked?
It sounds like the
> > > > > > callback needs to be synchronized with the kick.
> > > > >
> > > > > Note we only need to synchronize it when it changes,
which is
> > > > > only during initialization and reset.
> > > >
> > > > Yes.
> > > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > If driver does not kick then it's not
ready to get callbacks, right?
> > > > > > >
> > > > > > > Sounds quite clean, but we need to think
through memory ordering
> > > > > > > concerns - I guess it's only when we
change the value so
> > > > > > >         if (!vq->kicked) {
> > > > > > >                 vq->kicked = true;
> > > > > > >                 mb();
> > > > > > >         }
> > > > > > >
> > > > > > > will do the trick, right?
> > > > > >
> > > > > > There's no much difference with the existing
approach:
> > > > > >
> > > > > > 1) your proposal implicitly makes callbacks ready
in virtqueue_kick()
> > > > > > 2) my proposal explicitly makes callbacks ready
via virtio_device_ready()
> > > > > >
> > > > > > Both require careful auditing of all the existing
drivers to make sure
> > > > > > no kick before DRIVER_OK.
> > > > >
> > > > > Jason, kick before DRIVER_OK is out of spec, sure. But
it is unrelated
> > > > > to hardening
> > > >
> > > > Yes but with your proposal, it seems to couple kick with
DRIVER_OK somehow.
> > >
> > > I don't see how - my proposal ignores DRIVER_OK issues.
> >
> > Yes, what I meant is, in your proposal, the first kick after rest is a
> > hint that the driver is ok (but actually it could not).
> >
> > >
> > > > > and in absence of config interrupts is generally easily
> > > > > fixed just by sticking virtio_device_ready early in
initialization.
> > > >
> > > > So if the kick is done before the subsystem registration,
there's
> > > > still a window in the middle (assuming we stick
virtio_device_ready()
> > > > early):
> > > >
> > > > virtio_device_ready()
> > > > virtqueue_kick()
> > > > /* the window */
> > > > subsystem_registration()
> > >
> > > Absolutely, however, I do not think we really have many such
drivers
> > > since this has been known as a wrong thing to do since the
beginning.
> > > Want to try to find any?
> >
> > Yes, let me try and update.
> 
> This is basically the device that have an RX queue, so I've found the
> following drivers:
> 
> scmi, mac80211_hwsim, vsock, bt, balloon.
Looked and I don't see it yet. Let's consider
./net/vmw_vsock/virtio_transport.c for example. Assuming we block
callbacks until the first kick, what is the issue with probe exactly?

> >
> > >I couldn't ... except maybe bluetooth
> > > but that's just maintainer nacking fixes saying he'll fix
it
> > > his way ...
> > >
> > > > And during remove(), we get another window:
> > > >
> > > > subsysrem_unregistration()
> > > > /* the window */
> > > > virtio_device_reset()
> > >
> > > Same here.
> 
> Basically for the drivers that set driver_ok before registration,
I don't see what does driver_ok have to do with it.
> so
> we have a lot:
> 
> blk, net, mac80211_hwsim, scsi, vsock, bt, crypto, gpio, gpu, i2c,
> iommu, caif, pmem, input, mem
> 
> So I think there's no easy way to harden the notification without
> auditing the driver one by one (especially considering the driver may
> use bh or workqueue). The problem is the notification hardening
> depends on a correct or race-free probe/remove. So we need to fix the
> issues in probe/remove then do the hardening on the notification.
> 
> Thanks
So if drivers kick but are not ready to get callbacks then let's fix
that first of all, these are racy with existing qemu even ignoring
spec compliance.


-- 
MST

Jason Wang

2022-Jun-29 07:02 UTC

head link

[PATCH V3] virtio: disable notification hardening by default

On Wed, Jun 29, 2022 at 2:31 PM Michael S. Tsirkin <mst at redhat.com>
wrote:>
> On Wed, Jun 29, 2022 at 12:07:11PM +0800, Jason Wang wrote:
> > On Tue, Jun 28, 2022 at 2:17 PM Jason Wang <jasowang at
redhat.com> wrote:
> > >
> > > On Tue, Jun 28, 2022 at 1:00 PM Michael S. Tsirkin <mst at
redhat.com> wrote:
> > > >
> > > > On Tue, Jun 28, 2022 at 11:49:12AM +0800, Jason Wang wrote:
> > > > > > Heh. Yea sure. But things work fine for people.
What is the chance
> > > > > > your review found and fixed all driver bugs?
> > > > >
> > > > > I don't/can't audit all bugs but the race
between open/close against
> > > > > ready/reset. It looks to me a good chance to fix them
all but if you
> > > > > think differently, let me know
> > > > >
> > > > > > After two attempts
> > > > > > I don't feel like hoping audit will fix all
bugs.
> > > > >
> > > > > I've started the auditing and have 15+ patches in
the queue. (only
> > > > > covers bluetooth, console, pmem, virtio-net and caif).
Spotting the
> > > > > issue is not hard but the testing, It would take at
least the time of
> > > > > one release to finalize I guess.
> > > >
> > > > Absolutely. So I am looking for a way to implement hardening
that does
> > > > not break existing drivers.
> > >
> > > I totally agree with you to seek a way without bothering the
drivers.
> > > Just wonder if this is possbile.
> > >
> > > >
> > > >
> > > > > >
> > > > > >
> > > > > > > >
> > > > > > > > The reason config was kind of easy is
that config interrupt is rarely
> > > > > > > > vital for device function so arbitrarily
deferring that does not lead to
> > > > > > > > deadlocks - what you are trying to do
with VQ interrupts is
> > > > > > > > fundamentally different. Things are
especially bad if we just drop
> > > > > > > > an interrupt but deferring can lead to
problems too.
> > > > > > >
> > > > > > > I'm not sure I see the difference,
disable_irq() stuffs also delay the
> > > > > > > interrupt processing until enable_irq().
> > > > > >
> > > > > >
> > > > > > Absolutely. I am not at all sure disable_irq fixes
all problems.
> > > > > >
> > > > > > > >
> > > > > > > > Consider as an example
> > > > > > > >     virtio-net: fix race between
ndo_open() and virtio_device_ready()
> > > > > > > > if you just defer vq interrupts you get
deadlocks.
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > I don't see a deadlock here, maybe you
can show more detail on this?
> > > > > >
> > > > > > What I mean is this: if we revert the above
commit, things still
> > > > > > work (out of spec, but still). If we revert and
defer interrupts until
> > > > > > device ready then ndo_open that triggers before
device ready deadlocks.
> > > > >
> > > > > Ok, I guess you meant on a hypervisor that is strictly
written with spec.
> > > >
> > > > I mean on hypervisor that starts processing queues after
getting a kick
> > > > even without DRIVER_OK.
> > >
> > > Oh right.
> > >
> > > >
> > > > > >
> > > > > >
> > > > > > > >
> > > > > > > > So, thinking about all this, how about a
simple per vq flag meaning
> > > > > > > > "this vq was kicked since
reset"?
> > > > > > >
> > > > > > > And ignore the notification if vq is not
kicked? It sounds like the
> > > > > > > callback needs to be synchronized with the
kick.
> > > > > >
> > > > > > Note we only need to synchronize it when it
changes, which is
> > > > > > only during initialization and reset.
> > > > >
> > > > > Yes.
> > > > >
> > > > > >
> > > > > >
> > > > > > > >
> > > > > > > > If driver does not kick then it's
not ready to get callbacks, right?
> > > > > > > >
> > > > > > > > Sounds quite clean, but we need to think
through memory ordering
> > > > > > > > concerns - I guess it's only when we
change the value so
> > > > > > > >         if (!vq->kicked) {
> > > > > > > >                 vq->kicked = true;
> > > > > > > >                 mb();
> > > > > > > >         }
> > > > > > > >
> > > > > > > > will do the trick, right?
> > > > > > >
> > > > > > > There's no much difference with the
existing approach:
> > > > > > >
> > > > > > > 1) your proposal implicitly makes callbacks
ready in virtqueue_kick()
> > > > > > > 2) my proposal explicitly makes callbacks
ready via virtio_device_ready()
> > > > > > >
> > > > > > > Both require careful auditing of all the
existing drivers to make sure
> > > > > > > no kick before DRIVER_OK.
> > > > > >
> > > > > > Jason, kick before DRIVER_OK is out of spec, sure.
But it is unrelated
> > > > > > to hardening
> > > > >
> > > > > Yes but with your proposal, it seems to couple kick
with DRIVER_OK somehow.
> > > >
> > > > I don't see how - my proposal ignores DRIVER_OK issues.
> > >
> > > Yes, what I meant is, in your proposal, the first kick after rest
is a
> > > hint that the driver is ok (but actually it could not).
> > >
> > > >
> > > > > > and in absence of config interrupts is generally
easily
> > > > > > fixed just by sticking virtio_device_ready early
in initialization.
> > > > >
> > > > > So if the kick is done before the subsystem
registration, there's
> > > > > still a window in the middle (assuming we stick
virtio_device_ready()
> > > > > early):
> > > > >
> > > > > virtio_device_ready()
> > > > > virtqueue_kick()
> > > > > /* the window */
> > > > > subsystem_registration()
> > > >
> > > > Absolutely, however, I do not think we really have many such
drivers
> > > > since this has been known as a wrong thing to do since the
beginning.
> > > > Want to try to find any?
> > >
> > > Yes, let me try and update.
> >
> > This is basically the device that have an RX queue, so I've found
the
> > following drivers:
> >
> > scmi, mac80211_hwsim, vsock, bt, balloon.
>
> Looked and I don't see it yet. Let's consider
> ./net/vmw_vsock/virtio_transport.c for example. Assuming we block
> callbacks until the first kick, what is the issue with probe exactly?
We need to make sure the callback can survive when it runs before sub
system registration.
>
>
> > >
> > > >I couldn't ... except maybe bluetooth
> > > > but that's just maintainer nacking fixes saying
he'll fix it
> > > > his way ...
> > > >
> > > > > And during remove(), we get another window:
> > > > >
> > > > > subsysrem_unregistration()
> > > > > /* the window */
> > > > > virtio_device_reset()
> > > >
> > > > Same here.
> >
> > Basically for the drivers that set driver_ok before registration,
>
> I don't see what does driver_ok have to do with it.
I meant for those driver, in probe they do()

virtio_device_ready()
subsystem_register()

In remove() they do

subsystem_unregister()
virtio_device_reset()

for symmetry
>
> > so
> > we have a lot:
> >
> > blk, net, mac80211_hwsim, scsi, vsock, bt, crypto, gpio, gpu, i2c,
> > iommu, caif, pmem, input, mem
> >
> > So I think there's no easy way to harden the notification without
> > auditing the driver one by one (especially considering the driver may
> > use bh or workqueue). The problem is the notification hardening
> > depends on a correct or race-free probe/remove. So we need to fix the
> > issues in probe/remove then do the hardening on the notification.
> >
> > Thanks
>
> So if drivers kick but are not ready to get callbacks then let's fix
> that first of all, these are racy with existing qemu even ignoring
> spec compliance.
Yes, (the patches I've posted so far exist even with a well-behaved device).

Thanks
>
>
> --
> MST
>

Virtualization - Jun 2022 - [PATCH V3] virtio: disable notification hardening by default

[PATCH V3] virtio: disable notification hardening by default

[PATCH V3] virtio: disable notification hardening by default