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

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

[Yan Zhao]

guest writes to read-only memory regions need to be dropped.

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:
v5:
-changed write handler of ram device memory region from .write to
.write_with_attrs in patch 1 (Paolo)
(for vfio region in patch 2, I still keep the operations as .read & .write.
the reasons are:
1. vfio_region_ops are for mmio/pio regions. the top level read/write
dispatcher in kvm just ignores their return values. (the return value of
address_space_rw() is just ignored)
2. there are a lot of callers to vfio_region_read() and
vfio_region_write(), who actually do not care about the return values
)
-minor changes on text format in error logs.

v4:
-instead of modifying tracing log, added qemu_log_mask(LOG_GUEST_ERROR...)
to log guest writes to read-only regions (Philippe)

for
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 | 17 +++++++++++++++--
 memory.c         | 15 ++++++++++++---
 2 files changed, 27 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH v5 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.

Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Xin Zeng <xin.zeng@intel.com>
---
 memory.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/memory.c b/memory.c
index 601b749906..90a748912f 100644
--- a/memory.c
+++ b/memory.c
@@ -34,6 +34,7 @@
 #include "sysemu/accel.h"
 #include "hw/boards.h"
 #include "migration/vmstate.h"
+#include "qemu/log.h"
 
 //#define DEBUG_UNASSIGNED
 
@@ -1307,12 +1308,19 @@ static uint64_t memory_region_ram_device_read(void *opaque,
     return data;
 }
 
-static void memory_region_ram_device_write(void *opaque, hwaddr addr,
-                                           uint64_t data, unsigned size)
+static MemTxResult memory_region_ram_device_write(void *opaque, hwaddr addr,
+                                           uint64_t data, unsigned size,
+                                           MemTxAttrs attrs)
 {
     MemoryRegion *mr = opaque;
 
     trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data, size);
+    if (mr->readonly) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Invalid write to read only ram device region addr 0x%"
+                       HWADDR_PRIx" size %u\n", addr, size);
+        return MEMTX_ERROR;
+    }
 
     switch (size) {
     case 1:
@@ -1328,11 +1336,12 @@ static void memory_region_ram_device_write(void *opaque, hwaddr addr,
         *(uint64_t *)(mr->ram_block->host + addr) = data;
         break;
     }
+    return MEMTX_OK;
 }
 
 static const MemoryRegionOps ram_device_mem_ops = {
     .read = memory_region_ram_device_read,
-    .write = memory_region_ram_device_write,
+    .write_with_attrs = memory_region_ram_device_write,
     .endianness = DEVICE_HOST_ENDIAN,
     .valid = {
         .min_access_size = 1,
-- 
2.17.1

[PATCH v5 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.

Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.co>
Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Xin Zeng <xin.zeng@intel.com>
---
 hw/vfio/common.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 0b3593b3c0..2a4fedfeaa 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -38,6 +38,7 @@
 #include "sysemu/reset.h"
 #include "trace.h"
 #include "qapi/error.h"
+#include "qemu/log.h"
 
 VFIOGroupList vfio_group_list =
     QLIST_HEAD_INITIALIZER(vfio_group_list);
@@ -190,6 +191,16 @@ void vfio_region_write(void *opaque, hwaddr addr,
         uint64_t qword;
     } buf;
 
+    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
+    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Invalid write to read only vfio region (%s:region%d"
+                      "+0x%"HWADDR_PRIx" size %d)\n", vbasedev->name,
+                      region->nr, addr, size);
+
+        return;
+    }
+
     switch (size) {
     case 1:
         buf.byte = data;
@@ -215,8 +226,6 @@ void vfio_region_write(void *opaque, hwaddr addr,
                      addr, data, size);
     }
 
-    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
-
     /*
      * A read or write to a BAR always signals an INTx EOI.  This will
      * do nothing if not pending (including not in INTx mode).  We assume
-- 
2.17.1

[PATCH v5 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
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 2a4fedfeaa..bf510e66c0 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -980,6 +980,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 v5 2/3] hw/vfio: drop guest writes to ro regions

[Yan Zhao]

On Thu, Apr 30, 2020 at 03:02:36PM +0800, Philippe Mathieu-Daudé wrote:
> On 4/30/20 7:23 AM, Yan Zhao wrote:
> > for vfio regions that are without write permission,
> > drop guest writes to those regions.
> > 
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.co>
> 
> The full domain name is redhat.com.
>
oops. really sorry....

> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> > ---
> >   hw/vfio/common.c | 13 +++++++++++--
> >   1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index 0b3593b3c0..2a4fedfeaa 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -38,6 +38,7 @@
> >   #include "sysemu/reset.h"
> >   #include "trace.h"
> >   #include "qapi/error.h"
> > +#include "qemu/log.h"
> >   
> >   VFIOGroupList vfio_group_list =
> >       QLIST_HEAD_INITIALIZER(vfio_group_list);
> > @@ -190,6 +191,16 @@ void vfio_region_write(void *opaque, hwaddr addr,
> >           uint64_t qword;
> >       } buf;
> >   
> > +    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
> > +    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "Invalid write to read only vfio region (%s:region%d"
> > +                      "+0x%"HWADDR_PRIx" size %d)\n", vbasedev->name,
> > +                      region->nr, addr, size);
> > +
> > +        return;
> > +    }
> > +
> >       switch (size) {
> >       case 1:
> >           buf.byte = data;
> > @@ -215,8 +226,6 @@ void vfio_region_write(void *opaque, hwaddr addr,
> >                        addr, data, size);
> >       }
> >   
> > -    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
> > -
> >       /*
> >        * A read or write to a BAR always signals an INTx EOI.  This will
> >        * do nothing if not pending (including not in INTx mode).  We assume
> > 
> 

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

[Yan Zhao]

On Thu, Apr 30, 2020 at 03:07:21PM +0800, Philippe Mathieu-Daudé wrote:
> On 4/30/20 7:19 AM, Yan Zhao wrote:
> > for ram device regions, drop guest writes if the regions is read-only.
> > 
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> > ---
> >   memory.c | 15 ++++++++++++---
> >   1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/memory.c b/memory.c
> > index 601b749906..90a748912f 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -34,6 +34,7 @@
> >   #include "sysemu/accel.h"
> >   #include "hw/boards.h"
> >   #include "migration/vmstate.h"
> > +#include "qemu/log.h"
> >   
> >   //#define DEBUG_UNASSIGNED
> >   
> > @@ -1307,12 +1308,19 @@ static uint64_t memory_region_ram_device_read(void *opaque,
> >       return data;
> >   }
> >   
> > -static void memory_region_ram_device_write(void *opaque, hwaddr addr,
> > -                                           uint64_t data, unsigned size)
> > +static MemTxResult memory_region_ram_device_write(void *opaque, hwaddr addr,
> > +                                           uint64_t data, unsigned size,
> > +                                           MemTxAttrs attrs)
> 
> Style alignment is now of and can be adjusted easily.
> 
> >   {
> >       MemoryRegion *mr = opaque;
> >   
> >       trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data, size);
> > +    if (mr->readonly) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "Invalid write to read only ram device region addr 0x%"
> > +                       HWADDR_PRIx" size %u\n", addr, size);
> 
> Style alignment of here too.
> 
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks! I'll update it right now!

> 
> > +        return MEMTX_ERROR;
> > +    }
> >   
> >       switch (size) {
> >       case 1:
> > @@ -1328,11 +1336,12 @@ static void memory_region_ram_device_write(void *opaque, hwaddr addr,
> >           *(uint64_t *)(mr->ram_block->host + addr) = data;
> >           break;
> >       }
> > +    return MEMTX_OK;
> >   }
> >   
> >   static const MemoryRegionOps ram_device_mem_ops = {
> >       .read = memory_region_ram_device_read,
> > -    .write = memory_region_ram_device_write,
> > +    .write_with_attrs = memory_region_ram_device_write,
> >       .endianness = DEVICE_HOST_ENDIAN,
> >       .valid = {
> >           .min_access_size = 1,
> > 
> 

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

[Philippe Mathieu-Daudé]

On 4/30/20 7:23 AM, Yan Zhao wrote:
> for vfio regions that are without write permission,
> drop guest writes to those regions.
> 
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.co>

The full domain name is redhat.com.

> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> ---
>   hw/vfio/common.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 0b3593b3c0..2a4fedfeaa 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -38,6 +38,7 @@
>   #include "sysemu/reset.h"
>   #include "trace.h"
>   #include "qapi/error.h"
> +#include "qemu/log.h"
>   
>   VFIOGroupList vfio_group_list =
>       QLIST_HEAD_INITIALIZER(vfio_group_list);
> @@ -190,6 +191,16 @@ void vfio_region_write(void *opaque, hwaddr addr,
>           uint64_t qword;
>       } buf;
>   
> +    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
> +    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Invalid write to read only vfio region (%s:region%d"
> +                      "+0x%"HWADDR_PRIx" size %d)\n", vbasedev->name,
> +                      region->nr, addr, size);
> +
> +        return;
> +    }
> +
>       switch (size) {
>       case 1:
>           buf.byte = data;
> @@ -215,8 +226,6 @@ void vfio_region_write(void *opaque, hwaddr addr,
>                        addr, data, size);
>       }
>   
> -    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
> -
>       /*
>        * A read or write to a BAR always signals an INTx EOI.  This will
>        * do nothing if not pending (including not in INTx mode).  We assume
> 

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

[Philippe Mathieu-Daudé]

On 4/30/20 7:19 AM, Yan Zhao wrote:
> for ram device regions, drop guest writes if the regions is read-only.
> 
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> ---
>   memory.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 601b749906..90a748912f 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -34,6 +34,7 @@
>   #include "sysemu/accel.h"
>   #include "hw/boards.h"
>   #include "migration/vmstate.h"
> +#include "qemu/log.h"
>   
>   //#define DEBUG_UNASSIGNED
>   
> @@ -1307,12 +1308,19 @@ static uint64_t memory_region_ram_device_read(void *opaque,
>       return data;
>   }
>   
> -static void memory_region_ram_device_write(void *opaque, hwaddr addr,
> -                                           uint64_t data, unsigned size)
> +static MemTxResult memory_region_ram_device_write(void *opaque, hwaddr addr,
> +                                           uint64_t data, unsigned size,
> +                                           MemTxAttrs attrs)

Style alignment is now of and can be adjusted easily.

>   {
>       MemoryRegion *mr = opaque;
>   
>       trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data, size);
> +    if (mr->readonly) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Invalid write to read only ram device region addr 0x%"
> +                       HWADDR_PRIx" size %u\n", addr, size);

Style alignment of here too.

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

> +        return MEMTX_ERROR;
> +    }
>   
>       switch (size) {
>       case 1:
> @@ -1328,11 +1336,12 @@ static void memory_region_ram_device_write(void *opaque, hwaddr addr,
>           *(uint64_t *)(mr->ram_block->host + addr) = data;
>           break;
>       }
> +    return MEMTX_OK;
>   }
>   
>   static const MemoryRegionOps ram_device_mem_ops = {
>       .read = memory_region_ram_device_read,
> -    .write = memory_region_ram_device_write,
> +    .write_with_attrs = memory_region_ram_device_write,
>       .endianness = DEVICE_HOST_ENDIAN,
>       .valid = {
>           .min_access_size = 1,
>