linux-kernel - Re: [PATCH] virtio: break and reset virtio devices on device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACGkMEsK5Z_5B9Lz46kYYB_ndOG=CrvtZE_tLbRjL7=fT7OxbQ@mail.gmail.com>
Date: Fri, 21 Feb 2025 09:11:51 +0800
From: Jason Wang <jasowang@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: linux-kernel@...r.kernel.org, Eric Auger <eauger@...hat.com>, 
	Hongyu Ning <hongyu.ning@...ux.intel.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, 
	Eugenio Pérez <eperezma@...hat.com>, 
	virtualization@...ts.linux.dev
Subject: Re: [PATCH] virtio: break and reset virtio devices on device_shutdown()

On Fri, Feb 21, 2025 at 7:42 AM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> accesses during the hang.
>
>         Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
>         Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
>         ...
>
> It was traced down to virtio-console. Kexec works fine if virtio-console
> is not in use.
>
> The issue is that virtio-console continues to write to the MMIO even after
> underlying virtio-pci device is reset.

Some of my questions were not answered so I need to post them again.

Do we need to fix vitio-console?

Note that we've already break the device in virtio_pci_remove():

static void virtio_pci_remove(struct pci_dev *pci_dev)
{
  struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
        struct device *dev = get_device(&vp_dev->vdev.dev);

        /*
         * Device is marked broken on surprise removal so that virtio upper
         * layers can abort any ongoing operation.
         */
        if (!pci_device_is_present(pci_dev))
                virtio_break_device(&vp_dev->vdev);

...

>
> Additionally, Eric noticed that IOMMUs are reset before devices, if
> devices are not reset on shutdown they continue to poke at guest memory
> and get errors from the IOMMU. Some devices get wedged then.
>
> The problem can be solved by breaking all virtio devices on virtio
> bus shutdown, then resetting them.
>
> Reported-by: Eric Auger <eauger@...hat.com>
> Reported-by: Hongyu Ning <hongyu.ning@...ux.intel.com>
> Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
> ---
>  drivers/virtio/virtio.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index c1cc1157b380..e5b29520d3b2 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -377,6 +377,36 @@ static void virtio_dev_remove(struct device *_d)
>         of_node_put(dev->dev.of_node);
>  }
>
> +static void virtio_dev_shutdown(struct device *_d)
> +{
> +       struct virtio_device *dev = dev_to_virtio(_d);
> +       struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> +
> +       /*
> +        * Stop accesses to or from the device.
> +        * We only need to do it if there's a driver - no accesses otherwise.
> +        */
> +       if (!drv)
> +               return;
> +
> +       /*
> +        * Some devices get wedged if you kick them after they are
> +        * reset. Mark all vqs as broken to make sure we don't.
> +        */
> +       virtio_break_device(dev);
> +       /*
> +        * The below virtio_synchronize_cbs() guarantees that any interrupt
> +        * for this line arriving after virtio_synchronize_vqs() has completed
> +        * is guaranteed to see vq->broken as true.
> +        */
> +       virtio_synchronize_cbs(dev);

This looks like a partial re-introduction of the hardening work, but
the ccw part is still in-completed e.g the synchronization requires a
read_lock() and depends on CONFIG_VIRTIO_HARDEN_NOTIFICATION (which is
marked as broken now).

Should it better to

1) fix the virtio-console
2) simply rest in shutdown
3) wait for the hardening work to be done in the future?

Thanks

> +       /*
> +        * As IOMMUs are reset on shutdown, this will block device access to memory.
> +        * Some devices get wedged if this happens, so reset to make sure it does not.
> +        */
> +       dev->config->reset(dev);
> +}
> +
>  static const struct bus_type virtio_bus = {
>         .name  = "virtio",
>         .match = virtio_dev_match,
> @@ -384,6 +414,7 @@ static const struct bus_type virtio_bus = {
>         .uevent = virtio_uevent,
>         .probe = virtio_dev_probe,
>         .remove = virtio_dev_remove,
> +       .shutdown = virtio_dev_shutdown,
>  };
>
>  int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)
> --
> MST
>