[Qemu-devel] [PULL 0/1] pc: bugfix

[Qemu-devel] [PULL 0/1] pc: bugfix

[Michael S. Tsirkin]

The following changes since commit 27e757e29cc79f3f104d2a84d17cdb3b4c11c8ff:

  Update version for v2.12.0-rc4 release (2018-04-17 22:26:44 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to f256f05c94d12eed2c1a0e9b6ed4fb1fb9d8ec72:

  intel-iommu: send PSI always when notify_unmap set (2018-04-20 17:26:23 +0300)

----------------------------------------------------------------
pc: bugfix

Adds missing invalidations in vtd.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Peter Xu (1):
      intel-iommu: send PSI always when notify_unmap set

 hw/i386/intel_iommu.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

[Qemu-devel] [PULL 1/1] intel-iommu: send PSI always when notify_unmap set

[Michael S. Tsirkin]

From: Peter Xu <peterx@redhat.com>

During IOVA page table walk, there is a special case when:

- notify_unmap is set, meanwhile
- entry is invalid

In the past, we skip the entry always.  This is not correct.  We should
send UNMAP notification to registered notifiers in this case.  Otherwise
some stall pages will still be mapped in the host even if L1 guest
unmapped them already.

Without this patch, nested device assignment to L2 guests might dump
some errors like:

qemu-system-x86_64: VFIO_MAP_DMA: -17
qemu-system-x86_64: vfio_dma_map(0x557305420c30, 0xad000, 0x1000,
                    0x7f89a920d000) = -17 (File exists)

To fix this, we need to apply this patch to L1 QEMU (L2 QEMU is not
affected by this problem).

Signed-off-by: Peter Xu <peterx@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/intel_iommu.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index fb31de9..b359efd 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -722,6 +722,15 @@ static int vtd_iova_to_slpte(VTDContextEntry *ce, uint64_t iova, bool is_write,
 
 typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private);
 
+static int vtd_page_walk_one(IOMMUTLBEntry *entry, int level,
+                             vtd_page_walk_hook hook_fn, void *private)
+{
+    assert(hook_fn);
+    trace_vtd_page_walk_one(level, entry->iova, entry->translated_addr,
+                            entry->addr_mask, entry->perm);
+    return hook_fn(entry, private);
+}
+
 /**
  * vtd_page_walk_level - walk over specific level for IOVA range
  *
@@ -781,28 +790,37 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start,
          */
         entry_valid = read_cur | write_cur;
 
+        entry.target_as = &address_space_memory;
+        entry.iova = iova & subpage_mask;
+        entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
+        entry.addr_mask = ~subpage_mask;
+
         if (vtd_is_last_slpte(slpte, level)) {
-            entry.target_as = &address_space_memory;
-            entry.iova = iova & subpage_mask;
             /* NOTE: this is only meaningful if entry_valid == true */
             entry.translated_addr = vtd_get_slpte_addr(slpte, aw);
-            entry.addr_mask = ~subpage_mask;
-            entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur);
             if (!entry_valid && !notify_unmap) {
                 trace_vtd_page_walk_skip_perm(iova, iova_next);
                 goto next;
             }
-            trace_vtd_page_walk_one(level, entry.iova, entry.translated_addr,
-                                    entry.addr_mask, entry.perm);
-            if (hook_fn) {
-                ret = hook_fn(&entry, private);
-                if (ret < 0) {
-                    return ret;
-                }
+            ret = vtd_page_walk_one(&entry, level, hook_fn, private);
+            if (ret < 0) {
+                return ret;
             }
         } else {
             if (!entry_valid) {
-                trace_vtd_page_walk_skip_perm(iova, iova_next);
+                if (notify_unmap) {
+                    /*
+                     * The whole entry is invalid; unmap it all.
+                     * Translated address is meaningless, zero it.
+                     */
+                    entry.translated_addr = 0x0;
+                    ret = vtd_page_walk_one(&entry, level, hook_fn, private);
+                    if (ret < 0) {
+                        return ret;
+                    }
+                } else {
+                    trace_vtd_page_walk_skip_perm(iova, iova_next);
+                }
                 goto next;
             }
             ret = vtd_page_walk_level(vtd_get_slpte_addr(slpte, aw), iova,
-- 
MST

Re: [Qemu-devel] [PULL 0/1] pc: bugfix

[Peter Maydell]

On 20 April 2018 at 15:29, Michael S. Tsirkin <mst@redhat.com> wrote:
> The following changes since commit 27e757e29cc79f3f104d2a84d17cdb3b4c11c8ff:
>
>   Update version for v2.12.0-rc4 release (2018-04-17 22:26:44 +0100)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to f256f05c94d12eed2c1a0e9b6ed4fb1fb9d8ec72:
>
>   intel-iommu: send PSI always when notify_unmap set (2018-04-20 17:26:23 +0300)
>
> ----------------------------------------------------------------
> pc: bugfix
>
> Adds missing invalidations in vtd.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ----------------------------------------------------------------
> Peter Xu (1):
>       intel-iommu: send PSI always when notify_unmap set

You really can't send me a pull request at rc4 and expect
me to apply it without a very clear and detailed rationale
in the cover letter for why this bug fix justifies having
to roll an extra rc and delay the release. "bugfix" is
not sufficient, I'm afraid.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/1] pc: bugfix

[Michael S. Tsirkin]

On Fri, Apr 20, 2018 at 03:55:31PM +0100, Peter Maydell wrote:
> On 20 April 2018 at 15:29, Michael S. Tsirkin <mst@redhat.com> wrote:
> > The following changes since commit 27e757e29cc79f3f104d2a84d17cdb3b4c11c8ff:
> >
> >   Update version for v2.12.0-rc4 release (2018-04-17 22:26:44 +0100)
> >
> > are available in the git repository at:
> >
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> >
> > for you to fetch changes up to f256f05c94d12eed2c1a0e9b6ed4fb1fb9d8ec72:
> >
> >   intel-iommu: send PSI always when notify_unmap set (2018-04-20 17:26:23 +0300)
> >
> > ----------------------------------------------------------------
> > pc: bugfix
> >
> > Adds missing invalidations in vtd.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ----------------------------------------------------------------
> > Peter Xu (1):
> >       intel-iommu: send PSI always when notify_unmap set
> 
> You really can't send me a pull request at rc4 and expect
> me to apply it without a very clear and detailed rationale
> in the cover letter for why this bug fix justifies having
> to roll an extra rc and delay the release. "bugfix" is
> not sufficient, I'm afraid.
> 
> thanks
> -- PMM

Right, sorry about that.  It fixes a security problem unfortunately.
Would you like me to redo it with a more detailed description?

-- 
MST

Re: [Qemu-devel] [PULL 0/1] pc: bugfix

[Peter Maydell]

On 20 April 2018 at 16:06, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Apr 20, 2018 at 03:55:31PM +0100, Peter Maydell wrote:
>> On 20 April 2018 at 15:29, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > The following changes since commit 27e757e29cc79f3f104d2a84d17cdb3b4c11c8ff:
>> >
>> >   Update version for v2.12.0-rc4 release (2018-04-17 22:26:44 +0100)
>> >
>> > are available in the git repository at:
>> >
>> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>> >
>> > for you to fetch changes up to f256f05c94d12eed2c1a0e9b6ed4fb1fb9d8ec72:
>> >
>> >   intel-iommu: send PSI always when notify_unmap set (2018-04-20 17:26:23 +0300)
>> >
>> > ----------------------------------------------------------------
>> > pc: bugfix
>> >
>> > Adds missing invalidations in vtd.
>> >
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> >
>> > ----------------------------------------------------------------
>> > Peter Xu (1):
>> >       intel-iommu: send PSI always when notify_unmap set
>>
>> You really can't send me a pull request at rc4 and expect
>> me to apply it without a very clear and detailed rationale
>> in the cover letter for why this bug fix justifies having
>> to roll an extra rc and delay the release. "bugfix" is
>> not sufficient, I'm afraid.

> Right, sorry about that.  It fixes a security problem unfortunately.
> Would you like me to redo it with a more detailed description?

Would be nice. The commit message as it sounds certainly
doesn't sound like a security problem. Does this have a CVE?
If so the commit message should state it. Should it be cc:stable?
Is it a regression since 2.11? What exactly are the consequences
and who does it affect?

It's probably easier if you explain in email here first, and
then we can redo the commit message if it seems necessary.

thanks
-- PMM