We try to harden virtio device notifications in 8b4ec69d7e09 ("virtio:
harden vring IRQ"). It works with the assumption that the driver or
core can properly call virtio_device_ready() at the right
place. Unfortunately, this seems to be not true and uncover various
bugs of the existing drivers, mainly the issue of using
virtio_device_ready() incorrectly.
So let's having a Kconfig option and disable it by default. It gives
us a breath to fix the drivers and then we can consider to enable it
by default.
Signed-off-by: Jason Wang <[email protected]>
---
Changes since V1:
- tweak the Kconfig prompt
- don't hold spinlock for IRQ path in s390
---
drivers/s390/virtio/virtio_ccw.c | 4 ++++
drivers/virtio/Kconfig | 11 +++++++++++
drivers/virtio/virtio.c | 2 ++
drivers/virtio/virtio_ring.c | 12 ++++++++++++
include/linux/virtio_config.h | 2 ++
5 files changed, 31 insertions(+)
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 97e51c34e6cf..89bbf7ccfdd1 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -1136,8 +1136,10 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
vcdev->err = -EIO;
}
virtio_ccw_check_activity(vcdev, activity);
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
/* Interrupts are disabled here */
read_lock(&vcdev->irq_lock);
+#endif
for_each_set_bit(i, indicators(vcdev),
sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
/* The bit clear must happen before the vring kick. */
@@ -1146,7 +1148,9 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
vq = virtio_ccw_vq_by_ind(vcdev, i);
vring_interrupt(0, vq);
}
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
read_unlock(&vcdev->irq_lock);
+#endif
if (test_bit(0, indicators2(vcdev))) {
virtio_config_changed(&vcdev->vdev);
clear_bit(0, indicators2(vcdev));
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index b5adf6abd241..96ec56d44b91 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -35,6 +35,17 @@ menuconfig VIRTIO_MENU
if VIRTIO_MENU
+config VIRTIO_HARDEN_NOTIFICATION
+ bool "Harden virtio notification"
+ help
+ Enable this to harden the device notifications and supress
+ the ones that are illegal.
+
+ Experimental: not all drivers handle this correctly at this
+ point.
+
+ If unsure, say N.
+
config VIRTIO_PCI
tristate "PCI driver for virtio devices"
depends on PCI
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index ef04a96942bf..21dc08d2f32d 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -220,6 +220,7 @@ static int virtio_features_ok(struct virtio_device *dev)
* */
void virtio_reset_device(struct virtio_device *dev)
{
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
/*
* The below virtio_synchronize_cbs() guarantees that any
* interrupt for this line arriving after
@@ -228,6 +229,7 @@ void virtio_reset_device(struct virtio_device *dev)
*/
virtio_break_device(dev);
virtio_synchronize_cbs(dev);
+#endif
dev->config->reset(dev);
}
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 13a7348cedff..d9d3b6e201fb 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1688,7 +1688,11 @@ static struct virtqueue *vring_create_virtqueue_packed(
vq->we_own_ring = true;
vq->notify = notify;
vq->weak_barriers = weak_barriers;
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
vq->broken = true;
+#else
+ vq->broken = false;
+#endif
vq->last_used_idx = 0;
vq->event_triggered = false;
vq->num_added = 0;
@@ -2135,9 +2139,13 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
}
if (unlikely(vq->broken)) {
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
dev_warn_once(&vq->vq.vdev->dev,
"virtio vring IRQ raised before DRIVER_OK");
return IRQ_NONE;
+#else
+ return IRQ_HANDLED;
+#endif
}
/* Just a hint for performance: so it's ok that this can be racy! */
@@ -2180,7 +2188,11 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
vq->we_own_ring = false;
vq->notify = notify;
vq->weak_barriers = weak_barriers;
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
vq->broken = true;
+#else
+ vq->broken = false;
+#endif
vq->last_used_idx = 0;
vq->event_triggered = false;
vq->num_added = 0;
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 9a36051ceb76..d15c3cdda2d2 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -257,6 +257,7 @@ void virtio_device_ready(struct virtio_device *dev)
WARN_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
/*
* The virtio_synchronize_cbs() makes sure vring_interrupt()
* will see the driver specific setup if it sees vq->broken
@@ -264,6 +265,7 @@ void virtio_device_ready(struct virtio_device *dev)
*/
virtio_synchronize_cbs(dev);
__virtio_unbreak_device(dev);
+#endif
/*
* The transport should ensure the visibility of vq->broken
* before setting DRIVER_OK. See the comments for the transport
--
2.25.1
On Mon, Jun 20 2022, Jason Wang <[email protected]> wrote:
> We try to harden virtio device notifications in 8b4ec69d7e09 ("virtio:
> harden vring IRQ"). It works with the assumption that the driver or
> core can properly call virtio_device_ready() at the right
> place. Unfortunately, this seems to be not true and uncover various
> bugs of the existing drivers, mainly the issue of using
> virtio_device_ready() incorrectly.
>
> So let's having a Kconfig option and disable it by default. It gives
> us a breath to fix the drivers and then we can consider to enable it
> by default.
>
> Signed-off-by: Jason Wang <[email protected]>
> ---
> Changes since V1:
> - tweak the Kconfig prompt
> - don't hold spinlock for IRQ path in s390
> ---
> drivers/s390/virtio/virtio_ccw.c | 4 ++++
> drivers/virtio/Kconfig | 11 +++++++++++
> drivers/virtio/virtio.c | 2 ++
> drivers/virtio/virtio_ring.c | 12 ++++++++++++
> include/linux/virtio_config.h | 2 ++
> 5 files changed, 31 insertions(+)
>
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index 97e51c34e6cf..89bbf7ccfdd1 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -1136,8 +1136,10 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> vcdev->err = -EIO;
> }
> virtio_ccw_check_activity(vcdev, activity);
> +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> /* Interrupts are disabled here */
> read_lock(&vcdev->irq_lock);
Should we add a comment that this pairs with
virtio_ccw_synchronize_cbs()? Just to avoid future headscratching as to
why this lock is only needed when notification hardening is enabled.
> +#endif
> for_each_set_bit(i, indicators(vcdev),
> sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
> /* The bit clear must happen before the vring kick. */
> @@ -1146,7 +1148,9 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> vq = virtio_ccw_vq_by_ind(vcdev, i);
> vring_interrupt(0, vq);
> }
> +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> read_unlock(&vcdev->irq_lock);
> +#endif
> if (test_bit(0, indicators2(vcdev))) {
> virtio_config_changed(&vcdev->vdev);
> clear_bit(0, indicators2(vcdev));
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index b5adf6abd241..96ec56d44b91 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -35,6 +35,17 @@ menuconfig VIRTIO_MENU
>
> if VIRTIO_MENU
>
> +config VIRTIO_HARDEN_NOTIFICATION
> + bool "Harden virtio notification"
> + help
> + Enable this to harden the device notifications and supress
> + the ones that are illegal.
"...and suppress those that happen at a time where notifications are
illegal." ?
> +
> + Experimental: not all drivers handle this correctly at this
> + point.
"Note that several drivers still have bugs that may cause crashes or
hangs when correct handling of notifications is enforced; depending on
the subset of drivers and devices you use, this may or may not work."
Or is that too verbose?
> +
> + If unsure, say N.
> +
> config VIRTIO_PCI
> tristate "PCI driver for virtio devices"
> depends on PCI
The ifdeffery looks a big ugly, but I don't have a better idea.
On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <[email protected]> wrote:
>
> On Mon, Jun 20 2022, Jason Wang <[email protected]> wrote:
>
> > We try to harden virtio device notifications in 8b4ec69d7e09 ("virtio:
> > harden vring IRQ"). It works with the assumption that the driver or
> > core can properly call virtio_device_ready() at the right
> > place. Unfortunately, this seems to be not true and uncover various
> > bugs of the existing drivers, mainly the issue of using
> > virtio_device_ready() incorrectly.
> >
> > So let's having a Kconfig option and disable it by default. It gives
> > us a breath to fix the drivers and then we can consider to enable it
> > by default.
> >
> > Signed-off-by: Jason Wang <[email protected]>
> > ---
> > Changes since V1:
> > - tweak the Kconfig prompt
> > - don't hold spinlock for IRQ path in s390
> > ---
> > drivers/s390/virtio/virtio_ccw.c | 4 ++++
> > drivers/virtio/Kconfig | 11 +++++++++++
> > drivers/virtio/virtio.c | 2 ++
> > drivers/virtio/virtio_ring.c | 12 ++++++++++++
> > include/linux/virtio_config.h | 2 ++
> > 5 files changed, 31 insertions(+)
> >
> > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> > index 97e51c34e6cf..89bbf7ccfdd1 100644
> > --- a/drivers/s390/virtio/virtio_ccw.c
> > +++ b/drivers/s390/virtio/virtio_ccw.c
> > @@ -1136,8 +1136,10 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> > vcdev->err = -EIO;
> > }
> > virtio_ccw_check_activity(vcdev, activity);
> > +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> > /* Interrupts are disabled here */
> > read_lock(&vcdev->irq_lock);
>
> Should we add a comment that this pairs with
> virtio_ccw_synchronize_cbs()? Just to avoid future headscratching as to
> why this lock is only needed when notification hardening is enabled.
Fine.
>
> > +#endif
> > for_each_set_bit(i, indicators(vcdev),
> > sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
> > /* The bit clear must happen before the vring kick. */
> > @@ -1146,7 +1148,9 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> > vq = virtio_ccw_vq_by_ind(vcdev, i);
> > vring_interrupt(0, vq);
> > }
> > +#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> > read_unlock(&vcdev->irq_lock);
> > +#endif
> > if (test_bit(0, indicators2(vcdev))) {
> > virtio_config_changed(&vcdev->vdev);
> > clear_bit(0, indicators2(vcdev));
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index b5adf6abd241..96ec56d44b91 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -35,6 +35,17 @@ menuconfig VIRTIO_MENU
> >
> > if VIRTIO_MENU
> >
> > +config VIRTIO_HARDEN_NOTIFICATION
> > + bool "Harden virtio notification"
> > + help
> > + Enable this to harden the device notifications and supress
> > + the ones that are illegal.
>
> "...and suppress those that happen at a time where notifications are
> illegal." ?
Ok.
>
> > +
> > + Experimental: not all drivers handle this correctly at this
> > + point.
>
> "Note that several drivers still have bugs that may cause crashes or
> hangs when correct handling of notifications is enforced; depending on
> the subset of drivers and devices you use, this may or may not work."
>
> Or is that too verbose?
Looks fine.
>
> > +
> > + If unsure, say N.
> > +
> > config VIRTIO_PCI
> > tristate "PCI driver for virtio devices"
> > depends on PCI
>
> The ifdeffery looks a big ugly, but I don't have a better idea.
I guess you meant the ccw part, I leave the spinlock here in V1, but
Michael prefers to have that.
In the future, we may consider removing that, one possible way is to
have a per driver boolean for the hardening.
Tanks
>
On Tue, Jun 21 2022, Jason Wang <[email protected]> wrote:
> On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <[email protected]> wrote:
>>
>> The ifdeffery looks a big ugly, but I don't have a better idea.
>
> I guess you meant the ccw part, I leave the spinlock here in V1, but
> Michael prefers to have that.
Not doing the locking dance is good; I think the #ifdefs all over are a
bit ugly, but as I said, I can't think of a good, less-ugly way...
> In the future, we may consider removing that, one possible way is to
> have a per driver boolean for the hardening.
As in "we've reviewed and tested this driver, so let's turn it on for
every device bound to it"?
On Tue, Jun 21, 2022 at 5:58 PM Cornelia Huck <[email protected]> wrote:
>
> On Tue, Jun 21 2022, Jason Wang <[email protected]> wrote:
>
> > On Tue, Jun 21, 2022 at 5:16 PM Cornelia Huck <[email protected]> wrote:
> >>
> >> The ifdeffery looks a big ugly, but I don't have a better idea.
> >
> > I guess you meant the ccw part, I leave the spinlock here in V1, but
> > Michael prefers to have that.
>
> Not doing the locking dance is good; I think the #ifdefs all over are a
> bit ugly, but as I said, I can't think of a good, less-ugly way...
Probably, but this is the way that is used by other subsystems. E.g
CONFIG_HARDEN_USERCOPY etc.
>
> > In the future, we may consider removing that, one possible way is to
> > have a per driver boolean for the hardening.
>
> As in "we've reviewed and tested this driver, so let's turn it on for
> every device bound to it"?
Right.
Thanks
>