[PATCH v3 0/3] drop writes to read-only ram device & vfio regions

[PATCH v3 0/3] drop writes to read-only ram device & vfio regions

[Yan Zhao]

patch 1 modifies handler of ram device memory regions to drop guest writes
to read-only ram device memory regions

patch 2 modifies handler of non-mmap'd read-only vfio regions to drop guest
writes to those regions 

patch 3 set read-only flag to mmap'd read-only vfio regions, so that guest
writes to those regions would be trapped.
without patch 1, host qemu would then crash on guest write to those read-only
regions.
with patch 1, host qemu would drop the writes.

Changelog:
v3:
-refreshed and Cc Stefan for reviewing of tracing part

v2:
-split one big patches into smaller ones (Philippe)
-modify existing trace to record guest writes to read-only memory (Alex)
-modify vfio_region_write() to drop guest writes to non-mmap'd read-only
 region (Alex)

Yan Zhao (3):
  memory: drop guest writes to read-only ram device regions
  hw/vfio: drop guest writes to ro regions
  hw/vfio: let read-only flag take effect for mmap'd regions

 hw/vfio/common.c     | 12 +++++++++++-
 hw/vfio/trace-events |  2 +-
 memory.c             |  6 +++++-
 trace-events         |  2 +-
 4 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] memory: drop guest writes to read-only ram device regions

[Yan Zhao]

for ram device regions, drop guest writes if the regions is read-only.

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Xin Zeng <xin.zeng@intel.com>
---
 memory.c     | 6 +++++-
 trace-events | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 601b749906..a380b59980 100644
--- a/memory.c
+++ b/memory.c
@@ -1312,7 +1312,11 @@ static void memory_region_ram_device_write(void *opaque, hwaddr addr,
 {
     MemoryRegion *mr = opaque;
 
-    trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data, size);
+    trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data,
+                                         size, mr->readonly);
+    if (mr->readonly) {
+        return;
+    }
 
     switch (size) {
     case 1:
diff --git a/trace-events b/trace-events
index 42107ebc69..e1de662973 100644
--- a/trace-events
+++ b/trace-events
@@ -61,7 +61,7 @@ memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value,
 memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
-memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
+memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, bool readonly) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u" " is_readonly_region=%d"
 flatview_new(void *view, void *root) "%p (root %p)"
 flatview_destroy(void *view, void *root) "%p (root %p)"
 flatview_destroy_rcu(void *view, void *root) "%p (root %p)"
-- 
2.17.1

Re: [PATCH v3 1/3] memory: drop guest writes to read-only ram device regions

[Philippe Mathieu-Daudé]

On 4/13/20 8:37 AM, Yan Zhao wrote:
> for ram device regions, drop guest writes if the regions is read-only.
> 
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> ---
>   memory.c     | 6 +++++-
>   trace-events | 2 +-
>   2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 601b749906..a380b59980 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1312,7 +1312,11 @@ static void memory_region_ram_device_write(void *opaque, hwaddr addr,
>   {
>       MemoryRegion *mr = opaque;
>   
> -    trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data, size);
> +    trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data,
> +                                         size, mr->readonly);
> +    if (mr->readonly) {

            qemu_log_mask(LOG_GUEST_ERROR, ...)?

> +        return;
> +    }
>   
>       switch (size) {
>       case 1:
> diff --git a/trace-events b/trace-events
> index 42107ebc69..e1de662973 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -61,7 +61,7 @@ memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value,
>   memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
>   memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
>   memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
> -memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
> +memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, bool readonly) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u" " is_readonly_region=%d"
>   flatview_new(void *view, void *root) "%p (root %p)"
>   flatview_destroy(void *view, void *root) "%p (root %p)"
>   flatview_destroy_rcu(void *view, void *root) "%p (root %p)"
> 

Re: [PATCH v3 1/3] memory: drop guest writes to read-only ram device regions

[Yan Zhao]

On Tue, Apr 14, 2020 at 05:35:22PM +0800, Philippe Mathieu-Daudé wrote:
> On 4/13/20 8:37 AM, Yan Zhao wrote:
> > for ram device regions, drop guest writes if the regions is read-only.
> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> > ---
> >   memory.c     | 6 +++++-
> >   trace-events | 2 +-
> >   2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/memory.c b/memory.c
> > index 601b749906..a380b59980 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -1312,7 +1312,11 @@ static void memory_region_ram_device_write(void *opaque, hwaddr addr,
> >   {
> >       MemoryRegion *mr = opaque;
> >   
> > -    trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data, size);
> > +    trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data,
> > +                                         size, mr->readonly);
> > +    if (mr->readonly) {
> 
>             qemu_log_mask(LOG_GUEST_ERROR, ...)?
>
ok. will add it in next version.

Thanks
Yan

> > +        return;
> > +    }
> >   
> >       switch (size) {
> >       case 1:
> > diff --git a/trace-events b/trace-events
> > index 42107ebc69..e1de662973 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -61,7 +61,7 @@ memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value,
> >   memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
> >   memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
> >   memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
> > -memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
> > +memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, bool readonly) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u" " is_readonly_region=%d"
> >   flatview_new(void *view, void *root) "%p (root %p)"
> >   flatview_destroy(void *view, void *root) "%p (root %p)"
> >   flatview_destroy_rcu(void *view, void *root) "%p (root %p)"
> > 
> 

[PATCH v3 2/3] hw/vfio: drop guest writes to ro regions

[Yan Zhao]

for vfio regions that are without write permission,
drop guest writes to those regions.

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Xin Zeng <xin.zeng@intel.com>
---
 hw/vfio/common.c     | 8 +++++++-
 hw/vfio/trace-events | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 0b3593b3c0..fd6ee1fe3e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -190,6 +190,11 @@ void vfio_region_write(void *opaque, hwaddr addr,
         uint64_t qword;
     } buf;
 
+    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
+        trace_vfio_region_write(vbasedev->name, region->nr,
+                                   addr, data, size, true);
+        return;
+    }
     switch (size) {
     case 1:
         buf.byte = data;
@@ -215,7 +220,8 @@ void vfio_region_write(void *opaque, hwaddr addr,
                      addr, data, size);
     }
 
-    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
+    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size,
+                            false);
 
     /*
      * A read or write to a BAR always signals an INTx EOI.  This will
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index b1ef55a33f..fb9ff604e6 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -91,7 +91,7 @@ vfio_pci_nvlink2_setup_quirk_ssatgt(const char *name, uint64_t tgt, uint64_t siz
 vfio_pci_nvlink2_setup_quirk_lnkspd(const char *name, uint32_t link_speed) "%s link_speed=0x%x"
 
 # common.c
-vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)"
+vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size, bool readonly) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)" " is_readonly_region=%d."
 vfio_region_read(char *name, int index, uint64_t addr, unsigned size, uint64_t data) " (%s:region%d+0x%"PRIx64", %d) = 0x%"PRIx64
 vfio_iommu_map_notify(const char *op, uint64_t iova_start, uint64_t iova_end) "iommu %s @ 0x%"PRIx64" - 0x%"PRIx64
 vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add 0x%"PRIx64" - 0x%"PRIx64
-- 
2.17.1

Re: [PATCH v3 2/3] hw/vfio: drop guest writes to ro regions

[Philippe Mathieu-Daudé]

Hi Yan,

On 4/13/20 8:37 AM, Yan Zhao wrote:
> for vfio regions that are without write permission,
> drop guest writes to those regions.
> 
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> ---
>   hw/vfio/common.c     | 8 +++++++-
>   hw/vfio/trace-events | 2 +-
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 0b3593b3c0..fd6ee1fe3e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -190,6 +190,11 @@ void vfio_region_write(void *opaque, hwaddr addr,
>           uint64_t qword;
>       } buf;
>   

I'd move the trace here (trace always):

        trace_vfio_region_write(vbasedev->name, region->nr, addr, data, 
size);

> +    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
> +        trace_vfio_region_write(vbasedev->name, region->nr,
> +                                   addr, data, size, true);

And use qemu_log_mask(LOG_GUEST_ERROR, ...) here instead.

> +        return;
> +    }
>       switch (size) {
>       case 1:
>           buf.byte = data;
> @@ -215,7 +220,8 @@ void vfio_region_write(void *opaque, hwaddr addr,
>                        addr, data, size);
>       }
>   
> -    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
> +    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size,
> +                            false);
>   
>       /*
>        * A read or write to a BAR always signals an INTx EOI.  This will
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index b1ef55a33f..fb9ff604e6 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -91,7 +91,7 @@ vfio_pci_nvlink2_setup_quirk_ssatgt(const char *name, uint64_t tgt, uint64_t siz
>   vfio_pci_nvlink2_setup_quirk_lnkspd(const char *name, uint32_t link_speed) "%s link_speed=0x%x"
>   
>   # common.c
> -vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)"
> +vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size, bool readonly) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)" " is_readonly_region=%d."
>   vfio_region_read(char *name, int index, uint64_t addr, unsigned size, uint64_t data) " (%s:region%d+0x%"PRIx64", %d) = 0x%"PRIx64
>   vfio_iommu_map_notify(const char *op, uint64_t iova_start, uint64_t iova_end) "iommu %s @ 0x%"PRIx64" - 0x%"PRIx64
>   vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add 0x%"PRIx64" - 0x%"PRIx64
> 

Re: [PATCH v3 2/3] hw/vfio: drop guest writes to ro regions

[Yan Zhao]

On Tue, Apr 14, 2020 at 05:34:29PM +0800, Philippe Mathieu-Daudé wrote:
> Hi Yan,
> 
> On 4/13/20 8:37 AM, Yan Zhao wrote:
> > for vfio regions that are without write permission,
> > drop guest writes to those regions.
> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> > ---
> >   hw/vfio/common.c     | 8 +++++++-
> >   hw/vfio/trace-events | 2 +-
> >   2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index 0b3593b3c0..fd6ee1fe3e 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -190,6 +190,11 @@ void vfio_region_write(void *opaque, hwaddr addr,
> >           uint64_t qword;
> >       } buf;
> >   
> 
> I'd move the trace here (trace always):
> 
>         trace_vfio_region_write(vbasedev->name, region->nr, addr, data, 
> size);
> 
> > +    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
> > +        trace_vfio_region_write(vbasedev->name, region->nr,
> > +                                   addr, data, size, true);
> 
> And use qemu_log_mask(LOG_GUEST_ERROR, ...) here instead.
>

ok. will change it. Thanks!

Yan
> > +        return;
> > +    }
> >       switch (size) {
> >       case 1:
> >           buf.byte = data;
> > @@ -215,7 +220,8 @@ void vfio_region_write(void *opaque, hwaddr addr,
> >                        addr, data, size);
> >       }
> >   
> > -    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
> > +    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size,
> > +                            false);
> >   
> >       /*
> >        * A read or write to a BAR always signals an INTx EOI.  This will
> > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> > index b1ef55a33f..fb9ff604e6 100644
> > --- a/hw/vfio/trace-events
> > +++ b/hw/vfio/trace-events
> > @@ -91,7 +91,7 @@ vfio_pci_nvlink2_setup_quirk_ssatgt(const char *name, uint64_t tgt, uint64_t siz
> >   vfio_pci_nvlink2_setup_quirk_lnkspd(const char *name, uint32_t link_speed) "%s link_speed=0x%x"
> >   
> >   # common.c
> > -vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)"
> > +vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size, bool readonly) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)" " is_readonly_region=%d."
> >   vfio_region_read(char *name, int index, uint64_t addr, unsigned size, uint64_t data) " (%s:region%d+0x%"PRIx64", %d) = 0x%"PRIx64
> >   vfio_iommu_map_notify(const char *op, uint64_t iova_start, uint64_t iova_end) "iommu %s @ 0x%"PRIx64" - 0x%"PRIx64
> >   vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add 0x%"PRIx64" - 0x%"PRIx64
> > 
> 

[PATCH v3 3/3] hw/vfio: let read-only flag take effect for mmap'd regions

[Yan Zhao]

along side setting host page table to be read-only, the memory regions
are also required to be read-only, so that when guest writes to the
read-only & mmap'd regions, vmexits would happen and region write handlers
are called.

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Xin Zeng <xin.zeng@intel.com>
---
 hw/vfio/common.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index fd6ee1fe3e..fc7618e041 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -977,6 +977,10 @@ int vfio_region_mmap(VFIORegion *region)
                                           name, region->mmaps[i].size,
                                           region->mmaps[i].mmap);
         g_free(name);
+
+        if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
+            memory_region_set_readonly(&region->mmaps[i].mem, true);
+        }
         memory_region_add_subregion(region->mem, region->mmaps[i].offset,
                                     &region->mmaps[i].mem);
 
-- 
2.17.1

Re: [PATCH v3 3/3] hw/vfio: let read-only flag take effect for mmap'd regions

[Philippe Mathieu-Daudé]

On 4/13/20 8:37 AM, Yan Zhao wrote:
> along side setting host page table to be read-only, the memory regions
> are also required to be read-only, so that when guest writes to the
> read-only & mmap'd regions, vmexits would happen and region write handlers
> are called.
> 
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> ---
>   hw/vfio/common.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index fd6ee1fe3e..fc7618e041 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -977,6 +977,10 @@ int vfio_region_mmap(VFIORegion *region)
>                                             name, region->mmaps[i].size,
>                                             region->mmaps[i].mmap);
>           g_free(name);
> +
> +        if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
> +            memory_region_set_readonly(&region->mmaps[i].mem, true);
> +        }
>           memory_region_add_subregion(region->mem, region->mmaps[i].offset,
>                                       &region->mmaps[i].mem);
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 3/3] hw/vfio: let read-only flag take effect for mmap'd regions

[Yan Zhao]

On Tue, Apr 14, 2020 at 05:37:58PM +0800, Philippe Mathieu-Daudé wrote:
> On 4/13/20 8:37 AM, Yan Zhao wrote:
> > along side setting host page table to be read-only, the memory regions
> > are also required to be read-only, so that when guest writes to the
> > read-only & mmap'd regions, vmexits would happen and region write handlers
> > are called.
> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> > ---
> >   hw/vfio/common.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index fd6ee1fe3e..fc7618e041 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -977,6 +977,10 @@ int vfio_region_mmap(VFIORegion *region)
> >                                             name, region->mmaps[i].size,
> >                                             region->mmaps[i].mmap);
> >           g_free(name);
> > +
> > +        if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
> > +            memory_region_set_readonly(&region->mmaps[i].mem, true);
> > +        }
> >           memory_region_add_subregion(region->mem, region->mmaps[i].offset,
> >                                       &region->mmaps[i].mem);
> >   
> > 
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!