| Message ID | f3e9db7e-f3ab-1ae5-4da6-05dbd8d0206f@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2017年03月08日 11:21, Jason Wang wrote: > > On 2017年03月07日 18:55, Paolo Bonzini wrote: >> >> On 07/03/2017 09:47, Jason Wang wrote: >>> We don't destroy region cache during reset which can make the maps >>> of previous driver leaked to a buggy or malicious driver that don't >>> set vring address before starting to use the device. >> I'm still not sure as to how this can happen. Reset does clear >> desc/used/avail, which should then be checked before accessing the >> caches. > > But the code does not check them in fact? (E.g the attached qtest > patch can still pass check-qtest). > > Thanks Ok, the reproducer seems wrong. And I think what you mean is something like the check done in virtio_queue_ready(). But looks like not all virtqueue check for this. One example is virtio_net_handle_ctrl(), and there may be even more. So you want to fix them all? Thanks
----- Original Message ----- > From: "Jason Wang" <jasowang@redhat.com> > To: "Paolo Bonzini" <pbonzini@redhat.com>, mst@redhat.com, qemu-devel@nongnu.org > Cc: peterx@redhat.com > Sent: Wednesday, March 8, 2017 7:22:06 AM > Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset > > > > On 2017年03月08日 11:21, Jason Wang wrote: > > > > On 2017年03月07日 18:55, Paolo Bonzini wrote: > >> > >> On 07/03/2017 09:47, Jason Wang wrote: > >>> We don't destroy region cache during reset which can make the maps > >>> of previous driver leaked to a buggy or malicious driver that don't > >>> set vring address before starting to use the device. > >> I'm still not sure as to how this can happen. Reset does clear > >> desc/used/avail, which should then be checked before accessing the > >> caches. > > > > But the code does not check them in fact? (E.g the attached qtest > > patch can still pass check-qtest). > > > > Thanks > > Ok, the reproducer seems wrong. And I think what you mean is something > like the check done in virtio_queue_ready(). But looks like not all > virtqueue check for this. One example is virtio_net_handle_ctrl(), and > there may be even more. So you want to fix them all? Why would virtio_net_handle_ctrl be called when desc == 0? The checks are all in common virtio code. static void virtio_queue_notify_vq(VirtQueue *vq) { if (vq->vring.desc && vq->handle_output) { VirtIODevice *vdev = vq->vdev; if (unlikely(vdev->broken)) { return; } trace_virtio_queue_notify(vdev, vq - vdev->vq, vq); vq->handle_output(vdev, vq); } } 1440,29 55% Paolo
On Wed, 8 Mar 2017 14:22:06 +0800 Jason Wang <jasowang@redhat.com> wrote: > On 2017年03月08日 11:21, Jason Wang wrote: > > > > On 2017年03月07日 18:55, Paolo Bonzini wrote: > >> > >> On 07/03/2017 09:47, Jason Wang wrote: > >>> We don't destroy region cache during reset which can make the maps > >>> of previous driver leaked to a buggy or malicious driver that don't > >>> set vring address before starting to use the device. > >> I'm still not sure as to how this can happen. Reset does clear > >> desc/used/avail, which should then be checked before accessing the > >> caches. > > > > But the code does not check them in fact? (E.g the attached qtest > > patch can still pass check-qtest). > > > > Thanks > > Ok, the reproducer seems wrong. And I think what you mean is something > like the check done in virtio_queue_ready(). But looks like not all > virtqueue check for this. One example is virtio_net_handle_ctrl(), and Shouldn't the check for desc in virtio_queue_notify_vq() already take care of that? > there may be even more. So you want to fix them all? Obviously not speaking for Paolo, but I think the virtio core should have be audited for missing guards.
On 2017年03月08日 17:10, Paolo Bonzini wrote: > > ----- Original Message ----- >> From: "Jason Wang" <jasowang@redhat.com> >> To: "Paolo Bonzini" <pbonzini@redhat.com>, mst@redhat.com, qemu-devel@nongnu.org >> Cc: peterx@redhat.com >> Sent: Wednesday, March 8, 2017 7:22:06 AM >> Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset >> >> >> >> On 2017年03月08日 11:21, Jason Wang wrote: >>> On 2017年03月07日 18:55, Paolo Bonzini wrote: >>>> On 07/03/2017 09:47, Jason Wang wrote: >>>>> We don't destroy region cache during reset which can make the maps >>>>> of previous driver leaked to a buggy or malicious driver that don't >>>>> set vring address before starting to use the device. >>>> I'm still not sure as to how this can happen. Reset does clear >>>> desc/used/avail, which should then be checked before accessing the >>>> caches. >>> But the code does not check them in fact? (E.g the attached qtest >>> patch can still pass check-qtest). >>> >>> Thanks >> Ok, the reproducer seems wrong. And I think what you mean is something >> like the check done in virtio_queue_ready(). But looks like not all >> virtqueue check for this. One example is virtio_net_handle_ctrl(), and >> there may be even more. So you want to fix them all? > Why would virtio_net_handle_ctrl be called when desc == 0? The checks > are all in common virtio code. > > static void virtio_queue_notify_vq(VirtQueue *vq) > { > if (vq->vring.desc && vq->handle_output) { > VirtIODevice *vdev = vq->vdev; > > if (unlikely(vdev->broken)) { > return; > } > > trace_virtio_queue_notify(vdev, vq - vdev->vq, vq); > vq->handle_output(vdev, vq); > } > } > 1440,29 55% > Paolo Right, I miss this. But I find two possible leaks by auditing the callers of virtqueue_pop(): virtio_input_send() and virtio_balloon_set_status() which will call virtio_balloon_receive_stats(). Thanks
On 2017年03月08日 17:30, Cornelia Huck wrote: > On Wed, 8 Mar 2017 14:22:06 +0800 > Jason Wang <jasowang@redhat.com> wrote: > >> On 2017年03月08日 11:21, Jason Wang wrote: >>> On 2017年03月07日 18:55, Paolo Bonzini wrote: >>>> On 07/03/2017 09:47, Jason Wang wrote: >>>>> We don't destroy region cache during reset which can make the maps >>>>> of previous driver leaked to a buggy or malicious driver that don't >>>>> set vring address before starting to use the device. >>>> I'm still not sure as to how this can happen. Reset does clear >>>> desc/used/avail, which should then be checked before accessing the >>>> caches. >>> But the code does not check them in fact? (E.g the attached qtest >>> patch can still pass check-qtest). >>> >>> Thanks >> Ok, the reproducer seems wrong. And I think what you mean is something >> like the check done in virtio_queue_ready(). But looks like not all >> virtqueue check for this. One example is virtio_net_handle_ctrl(), and > Shouldn't the check for desc in virtio_queue_notify_vq() already take > care of that? Yes, I miss this. > >> there may be even more. So you want to fix them all? > Obviously not speaking for Paolo, but I think the virtio core should > have be audited for missing guards. > Probably, otherwise we need a careful auditing of all callers of caches. Thanks
On Wed, 8 Mar 2017 17:53:23 +0800 Jason Wang <jasowang@redhat.com> wrote: > On 2017年03月08日 17:30, Cornelia Huck wrote: > > On Wed, 8 Mar 2017 14:22:06 +0800 > > Jason Wang <jasowang@redhat.com> wrote: > >> there may be even more. So you want to fix them all? > > Obviously not speaking for Paolo, but I think the virtio core should > > have be audited for missing guards. > > > > Probably, otherwise we need a careful auditing of all callers of caches. Yes, especially as all direct calls to the caches are localized in the core anyway.
On 08/03/2017 10:48, Jason Wang wrote: > > > On 2017年03月08日 17:10, Paolo Bonzini wrote: >> >> ----- Original Message ----- >>> From: "Jason Wang" <jasowang@redhat.com> >>> To: "Paolo Bonzini" <pbonzini@redhat.com>, mst@redhat.com, >>> qemu-devel@nongnu.org >>> Cc: peterx@redhat.com >>> Sent: Wednesday, March 8, 2017 7:22:06 AM >>> Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during >>> reset >>> >>> >>> >>> On 2017年03月08日 11:21, Jason Wang wrote: >>>> On 2017年03月07日 18:55, Paolo Bonzini wrote: >>>>> On 07/03/2017 09:47, Jason Wang wrote: >>>>>> We don't destroy region cache during reset which can make the maps >>>>>> of previous driver leaked to a buggy or malicious driver that don't >>>>>> set vring address before starting to use the device. >>>>> I'm still not sure as to how this can happen. Reset does clear >>>>> desc/used/avail, which should then be checked before accessing the >>>>> caches. >>>> But the code does not check them in fact? (E.g the attached qtest >>>> patch can still pass check-qtest). >>>> >>>> Thanks >>> Ok, the reproducer seems wrong. And I think what you mean is something >>> like the check done in virtio_queue_ready(). But looks like not all >>> virtqueue check for this. One example is virtio_net_handle_ctrl(), and >>> there may be even more. So you want to fix them all? >> Why would virtio_net_handle_ctrl be called when desc == 0? The checks >> are all in common virtio code. > > Right, I miss this. > > But I find two possible leaks by auditing the callers of virtqueue_pop(): > > virtio_input_send() and virtio_balloon_set_status() which will call > virtio_balloon_receive_stats(). Ok, this is good! I think we should check vq->vring.desc in virtio_queue_empty and virtio_queue_empty_rcu, and that should do it. It will cover virtqueue_pop too. virtqueue_get_avail_bytes is always preceded or followed by virtio_queue_empty, virtio_queue_ready or virtqueue_pop, but perhaps we should add a check there too. Paolo Paolo
On Thu, 9 Mar 2017 12:10:44 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 08/03/2017 10:48, Jason Wang wrote: > > > > > > On 2017年03月08日 17:10, Paolo Bonzini wrote: > >> > >> ----- Original Message ----- > >>> From: "Jason Wang" <jasowang@redhat.com> > >>> To: "Paolo Bonzini" <pbonzini@redhat.com>, mst@redhat.com, > >>> qemu-devel@nongnu.org > >>> Cc: peterx@redhat.com > >>> Sent: Wednesday, March 8, 2017 7:22:06 AM > >>> Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during > >>> reset > >>> > >>> > >>> > >>> On 2017年03月08日 11:21, Jason Wang wrote: > >>>> On 2017年03月07日 18:55, Paolo Bonzini wrote: > >>>>> On 07/03/2017 09:47, Jason Wang wrote: > >>>>>> We don't destroy region cache during reset which can make the maps > >>>>>> of previous driver leaked to a buggy or malicious driver that don't > >>>>>> set vring address before starting to use the device. > >>>>> I'm still not sure as to how this can happen. Reset does clear > >>>>> desc/used/avail, which should then be checked before accessing the > >>>>> caches. > >>>> But the code does not check them in fact? (E.g the attached qtest > >>>> patch can still pass check-qtest). > >>>> > >>>> Thanks > >>> Ok, the reproducer seems wrong. And I think what you mean is something > >>> like the check done in virtio_queue_ready(). But looks like not all > >>> virtqueue check for this. One example is virtio_net_handle_ctrl(), and > >>> there may be even more. So you want to fix them all? > >> Why would virtio_net_handle_ctrl be called when desc == 0? The checks > >> are all in common virtio code. > > > > > Right, I miss this. > > > > But I find two possible leaks by auditing the callers of virtqueue_pop(): > > > > virtio_input_send() and virtio_balloon_set_status() which will call > > virtio_balloon_receive_stats(). > > Ok, this is good! I think we should check vq->vring.desc in > virtio_queue_empty and virtio_queue_empty_rcu, and that should do it. > It will cover virtqueue_pop too. Yes, virtio_queue_empty{_rcu} should check for !desc. > virtqueue_get_avail_bytes is always preceded or followed by > virtio_queue_empty, virtio_queue_ready or virtqueue_pop, but perhaps we > should add a check there too. virtqueue_get_avail_bytes is an exported interface, so I think it should check as well. virtqueue_fill and virtqueue_flush as well, I think. Better safe than sorry.
From 8f588662d439c74e4da3924464167d2be330fd66 Mon Sep 17 00:00:00 2001 From: Jason Wang <jasowang@redhat.com> Date: Wed, 8 Mar 2017 11:19:37 +0800 Subject: [PATCH] virtio-net: qtest for region cache Signed-off-by: Jason Wang <jasowang@redhat.com> --- tests/libqos/virtio-pci.c | 21 ++++++++++++++++++--- tests/libqos/virtio.c | 8 ++++++++ tests/libqos/virtio.h | 6 ++++++ tests/virtio-net-test.c | 3 +++ 4 files changed, 35 insertions(+), 3 deletions(-) diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c index 7ac15c0..6273635 100644 --- a/tests/libqos/virtio-pci.c +++ b/tests/libqos/virtio-pci.c @@ -222,8 +222,9 @@ static void qvirtio_pci_set_queue_address(QVirtioDevice *d, uint32_t pfn) qpci_io_writel(dev->pdev, dev->bar, VIRTIO_PCI_QUEUE_PFN, pfn); } -static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d, - QGuestAllocator *alloc, uint16_t index) +static QVirtQueue *__qvirtio_pci_virtqueue_setup(QVirtioDevice *d, + QGuestAllocator *alloc, uint16_t index, + bool set_pfn) { uint32_t feat; uint64_t addr; @@ -254,11 +255,24 @@ static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d, addr = guest_alloc(alloc, qvring_size(vqpci->vq.size, VIRTIO_PCI_VRING_ALIGN)); qvring_init(alloc, &vqpci->vq, addr); - qvirtio_pci_set_queue_address(d, vqpci->vq.desc / VIRTIO_PCI_VRING_ALIGN); + if (set_pfn) + qvirtio_pci_set_queue_address(d, vqpci->vq.desc / VIRTIO_PCI_VRING_ALIGN); return &vqpci->vq; } +static QVirtQueue *qvirtio_pci_virtqueue_setup(QVirtioDevice *d, + QGuestAllocator *alloc, uint16_t index) +{ + return __qvirtio_pci_virtqueue_setup(d, alloc, index, true); +} + +static QVirtQueue *qvirtio_pci_virtqueue_setup2(QVirtioDevice *d, + QGuestAllocator *alloc, uint16_t index) +{ + return __qvirtio_pci_virtqueue_setup(d, alloc, index, false); +} + static void qvirtio_pci_virtqueue_cleanup(QVirtQueue *vq, QGuestAllocator *alloc) { @@ -290,6 +304,7 @@ const QVirtioBus qvirtio_pci = { .get_queue_size = qvirtio_pci_get_queue_size, .set_queue_address = qvirtio_pci_set_queue_address, .virtqueue_setup = qvirtio_pci_virtqueue_setup, + .virtqueue_setup2 = qvirtio_pci_virtqueue_setup2, .virtqueue_cleanup = qvirtio_pci_virtqueue_cleanup, .virtqueue_kick = qvirtio_pci_virtqueue_kick, }; diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c index ec30cb9..def910f 100644 --- a/tests/libqos/virtio.c +++ b/tests/libqos/virtio.c @@ -49,6 +49,12 @@ QVirtQueue *qvirtqueue_setup(QVirtioDevice *d, return d->bus->virtqueue_setup(d, alloc, index); } +QVirtQueue *qvirtqueue_setup2(QVirtioDevice *d, + QGuestAllocator *alloc, uint16_t index) +{ + return d->bus->virtqueue_setup(d, alloc, index); +} + void qvirtqueue_cleanup(const QVirtioBus *bus, QVirtQueue *vq, QGuestAllocator *alloc) { @@ -77,8 +83,10 @@ void qvirtio_set_driver(QVirtioDevice *d) void qvirtio_set_driver_ok(QVirtioDevice *d) { d->bus->set_status(d, d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK); + #if 0 g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK | VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE); + #endif } void qvirtio_wait_queue_isr(QVirtioDevice *d, diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h index 3397a08..2c194b9 100644 --- a/tests/libqos/virtio.h +++ b/tests/libqos/virtio.h @@ -82,6 +82,10 @@ struct QVirtioBus { QVirtQueue *(*virtqueue_setup)(QVirtioDevice *d, QGuestAllocator *alloc, uint16_t index); + /* Setup the virtqueue specified by index */ + QVirtQueue *(*virtqueue_setup2)(QVirtioDevice *d, QGuestAllocator *alloc, + uint16_t index); + /* Free virtqueue resources */ void (*virtqueue_cleanup)(QVirtQueue *vq, QGuestAllocator *alloc); @@ -123,6 +127,8 @@ uint8_t qvirtio_wait_status_byte_no_isr(QVirtioDevice *d, void qvirtio_wait_config_isr(QVirtioDevice *d, gint64 timeout_us); QVirtQueue *qvirtqueue_setup(QVirtioDevice *d, QGuestAllocator *alloc, uint16_t index); +QVirtQueue *qvirtqueue_setup2(QVirtioDevice *d, + QGuestAllocator *alloc, uint16_t index); void qvirtqueue_cleanup(const QVirtioBus *bus, QVirtQueue *vq, QGuestAllocator *alloc); diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c index 8f94360..df54b19 100644 --- a/tests/virtio-net-test.c +++ b/tests/virtio-net-test.c @@ -224,6 +224,9 @@ static void pci_basic(gconstpointer data) rx = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 0); tx = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 1); + qvirtio_reset(&dev->vdev); + rx = (QVirtQueuePCI *)qvirtqueue_setup2(&dev->vdev, qs->alloc, 0); + tx = (QVirtQueuePCI *)qvirtqueue_setup2(&dev->vdev, qs->alloc, 1); driver_init(&dev->vdev); func(&dev->vdev, qs->alloc, &rx->vq, &tx->vq, sv[0]); -- 2.7.4