diff mbox series

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

Message ID 20200417074437.28526-1-yan.y.zhao@intel.com
State New
Headers show
Series drop writes to read-only ram device & vfio regions | expand

Commit Message

Yan Zhao April 17, 2020, 7:44 a.m. UTC 
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 | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Philippe Mathieu-Daudé April 17, 2020, 8:48 a.m. UTC | #1 
On 4/17/20 9:44 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 | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/memory.c b/memory.c
> index 601b749906..9576dd6807 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
>   
> @@ -1313,6 +1314,12 @@ 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);
> +    if (mr->readonly) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Invalid write to read only ram device region 0x%"
> +                       HWADDR_PRIx" size %u\n", addr, size);
> +        return;
> +    }
>   
>       switch (size) {
>       case 1:
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Paolo Bonzini April 25, 2020, 10:55 a.m. UTC | #2 
On 17/04/20 09:44, 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/memory.c b/memory.c
> index 601b749906..9576dd6807 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
>  
> @@ -1313,6 +1314,12 @@ 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);
> +    if (mr->readonly) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Invalid write to read only ram device region 0x%"
> +                       HWADDR_PRIx" size %u\n", addr, size);
> +        return;
> +    }

As mentioned in the review of v1, memory_region_ram_device_write should
be changed to a .write_with_attrs operation, so that it can return
MEMTX_ERROR.

Otherwise this looks good.

Paolo
Yan Zhao April 26, 2020, 1:04 a.m. UTC | #3 
On Sat, Apr 25, 2020 at 06:55:33PM +0800, Paolo Bonzini wrote:
> On 17/04/20 09:44, 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 | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/memory.c b/memory.c
> > index 601b749906..9576dd6807 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
> >  
> > @@ -1313,6 +1314,12 @@ 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);
> > +    if (mr->readonly) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "Invalid write to read only ram device region 0x%"
> > +                       HWADDR_PRIx" size %u\n", addr, size);
> > +        return;
> > +    }
> 
> As mentioned in the review of v1, memory_region_ram_device_write should
> be changed to a .write_with_attrs operation, so that it can return
> MEMTX_ERROR.
> 
> Otherwise this looks good.
> 
hi Paolo,
thanks for pointing it out again!
I didn't get your meaning in v1. will update the patch!

Thanks
Yan
>
Yan Zhao April 27, 2020, 9:15 a.m. UTC | #4 
On Sun, Apr 26, 2020 at 09:04:31AM +0800, Yan Zhao wrote:
> On Sat, Apr 25, 2020 at 06:55:33PM +0800, Paolo Bonzini wrote:
> > On 17/04/20 09:44, 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 | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/memory.c b/memory.c
> > > index 601b749906..9576dd6807 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
> > >  
> > > @@ -1313,6 +1314,12 @@ 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);
> > > +    if (mr->readonly) {
> > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > +                      "Invalid write to read only ram device region 0x%"
> > > +                       HWADDR_PRIx" size %u\n", addr, size);
> > > +        return;
> > > +    }
> > 
> > As mentioned in the review of v1, memory_region_ram_device_write should
> > be changed to a .write_with_attrs operation, so that it can return
> > MEMTX_ERROR.
> >
hi Paolo and Alex,
need I also change vfio_region_write() in patch 2 to a .write_with_attrs
operation?
vfio_region_read() is also possible to fail, so should I change it to a
.read_with_attrs, too?

Thanks
Yan

> > Otherwise this looks good.
> > 
> hi Paolo,
> thanks for pointing it out again!
> I didn't get your meaning in v1. will update the patch!
> 
> Thanks
> Yan
> > 
>
Philippe Mathieu-Daudé April 27, 2020, 9:31 a.m. UTC | #5 
On 4/27/20 11:15 AM, Yan Zhao wrote:
> On Sun, Apr 26, 2020 at 09:04:31AM +0800, Yan Zhao wrote:
>> On Sat, Apr 25, 2020 at 06:55:33PM +0800, Paolo Bonzini wrote:
>>> On 17/04/20 09:44, 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 | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/memory.c b/memory.c
>>>> index 601b749906..9576dd6807 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
>>>>   
>>>> @@ -1313,6 +1314,12 @@ 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);
>>>> +    if (mr->readonly) {
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "Invalid write to read only ram device region 0x%"
>>>> +                       HWADDR_PRIx" size %u\n", addr, size);
>>>> +        return;
>>>> +    }
>>>
>>> As mentioned in the review of v1, memory_region_ram_device_write should
>>> be changed to a .write_with_attrs operation, so that it can return
>>> MEMTX_ERROR.
>>>
> hi Paolo and Alex,
> need I also change vfio_region_write() in patch 2 to a .write_with_attrs
> operation?
> vfio_region_read() is also possible to fail, so should I change it to a
> .read_with_attrs, too?

Yes.

Please submit your series as a thread, with a cover letter:
https://wiki.qemu.org/Contribute/SubmitAPatch#Include_a_meaningful_cover_letter

> 
> Thanks
> Yan
> 
>>> Otherwise this looks good.
>>>
>> hi Paolo,
>> thanks for pointing it out again!
>> I didn't get your meaning in v1. will update the patch!
>>
>> Thanks
>> Yan
>>>
>>
>
Yan Zhao April 27, 2020, 10:09 a.m. UTC | #6 
On Mon, Apr 27, 2020 at 05:31:48PM +0800, Philippe Mathieu-Daudé wrote:
> On 4/27/20 11:15 AM, Yan Zhao wrote:
> > On Sun, Apr 26, 2020 at 09:04:31AM +0800, Yan Zhao wrote:
> >> On Sat, Apr 25, 2020 at 06:55:33PM +0800, Paolo Bonzini wrote:
> >>> On 17/04/20 09:44, 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 | 7 +++++++
> >>>>   1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/memory.c b/memory.c
> >>>> index 601b749906..9576dd6807 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
> >>>>   
> >>>> @@ -1313,6 +1314,12 @@ 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);
> >>>> +    if (mr->readonly) {
> >>>> +        qemu_log_mask(LOG_GUEST_ERROR,
> >>>> +                      "Invalid write to read only ram device region 0x%"
> >>>> +                       HWADDR_PRIx" size %u\n", addr, size);
> >>>> +        return;
> >>>> +    }
> >>>
> >>> As mentioned in the review of v1, memory_region_ram_device_write should
> >>> be changed to a .write_with_attrs operation, so that it can return
> >>> MEMTX_ERROR.
> >>>
> > hi Paolo and Alex,
> > need I also change vfio_region_write() in patch 2 to a .write_with_attrs
> > operation?
> > vfio_region_read() is also possible to fail, so should I change it to a
> > .read_with_attrs, too?
> 
> Yes.
> 
> Please submit your series as a thread, with a cover letter:
> https://wiki.qemu.org/Contribute/SubmitAPatch#Include_a_meaningful_cover_letter
>
hi Philippe
thanks for pointing out this issue.
I just realized this version of patches were sent separately, though I did send
a cover letter. not sure what happened. maybe I just forgot to add an
-in-reply-to before sending out.
will pay attention to it next time.

Thanks
Yan

> > 
> > Thanks
> > Yan
> > 
> >>> Otherwise this looks good.
> >>>
> >> hi Paolo,
> >> thanks for pointing it out again!
> >> I didn't get your meaning in v1. will update the patch!
> >>
> >> Thanks
> >> Yan
> >>>
> >>
> > 
>
diff mbox series

Patch

diff --git a/memory.c b/memory.c
index 601b749906..9576dd6807 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
 
@@ -1313,6 +1314,12 @@  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);
+    if (mr->readonly) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Invalid write to read only ram device region 0x%"
+                       HWADDR_PRIx" size %u\n", addr, size);
+        return;
+    }
 
     switch (size) {
     case 1: