In case of the virtio-blk communication, can get the following assertion
for the specifically crafted virtio packet:
  qemu-system-x86_64: exec.c:3725: address_space_unmap: Assertion `mr !=
  NULL' failed.
This assertion is triggered if the length of the first descriptor in the
block request chain (block command descriptor) is more than block command
size. In this case the hw/block/virtio-blk.c:virtio_blk_handle_request()
routine calls the iov_discard_front() function and the iov base and size
are changed. As a result the address can not be found during the
address_space_unmap() call.

The fix is to check the whole address range in the address_space_unmap
function.

Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
---
 exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 86a38d3..0eeb018 100644
--- a/exec.c
+++ b/exec.c
@@ -3717,7 +3717,7 @@ void *address_space_map(AddressSpace *as,
 void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
                          int is_write, hwaddr access_len)
 {
-    if (buffer != bounce.buffer) {
+    if ((buffer < bounce.buffer) || (buffer + access_len > bounce.buffer + bounce.len)) {
         MemoryRegion *mr;
         ram_addr_t addr1;
 
-- 
2.7.4

On Fri, 22 Mar 2019 at 13:19, Dima Stepanov <dimastep@yandex-team.ru> wrote:
>
> In case of the virtio-blk communication, can get the following assertion
> for the specifically crafted virtio packet:
>   qemu-system-x86_64: exec.c:3725: address_space_unmap: Assertion `mr !=
>   NULL' failed.
> This assertion is triggered if the length of the first descriptor in the
> block request chain (block command descriptor) is more than block command
> size. In this case the hw/block/virtio-blk.c:virtio_blk_handle_request()
> routine calls the iov_discard_front() function and the iov base and size
> are changed. As a result the address can not be found during the
> address_space_unmap() call.
>
> The fix is to check the whole address range in the address_space_unmap
> function.
>
> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> ---
>  exec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/exec.c b/exec.c
> index 86a38d3..0eeb018 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3717,7 +3717,7 @@ void *address_space_map(AddressSpace *as,
>  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>                           int is_write, hwaddr access_len)
>  {
> -    if (buffer != bounce.buffer) {
> +    if ((buffer < bounce.buffer) || (buffer + access_len > bounce.buffer + bounce.len)) {
>          MemoryRegion *mr;
>          ram_addr_t addr1;

A quick look at the xen_invalidate_map_cache_entry() implementation
suggests that it also assumes that the address passed to
address_space_unmap() must be the same address that was
originally handed out via address_space_map().

So I think we either need to also change the Xen code, or
we need to fix this at the virtio level by having it keep
track of the buffer it was handed so it can unmap it.

thanks
-- PMM

On Fri, Mar 22, 2019 at 01:35:57PM +0000, Peter Maydell wrote:
> On Fri, 22 Mar 2019 at 13:19, Dima Stepanov <dimastep@yandex-team.ru> wrote:
> >
> > In case of the virtio-blk communication, can get the following assertion
> > for the specifically crafted virtio packet:
> >   qemu-system-x86_64: exec.c:3725: address_space_unmap: Assertion `mr !=
> >   NULL' failed.
> > This assertion is triggered if the length of the first descriptor in the
> > block request chain (block command descriptor) is more than block command
> > size. In this case the hw/block/virtio-blk.c:virtio_blk_handle_request()
> > routine calls the iov_discard_front() function and the iov base and size
> > are changed. As a result the address can not be found during the
> > address_space_unmap() call.
> >
> > The fix is to check the whole address range in the address_space_unmap
> > function.
> >
> > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> > ---
> >  exec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/exec.c b/exec.c
> > index 86a38d3..0eeb018 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -3717,7 +3717,7 @@ void *address_space_map(AddressSpace *as,
> >  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
> >                           int is_write, hwaddr access_len)
> >  {
> > -    if (buffer != bounce.buffer) {
> > +    if ((buffer < bounce.buffer) || (buffer + access_len > bounce.buffer + bounce.len)) {
> >          MemoryRegion *mr;
> >          ram_addr_t addr1;
> 
> A quick look at the xen_invalidate_map_cache_entry() implementation
> suggests that it also assumes that the address passed to
> address_space_unmap() must be the same address that was
> originally handed out via address_space_map().
Hard to say for me, if it is needed or not, since we have no xen
reproducer for this issue. Right now we are making some fuzzing for the
virtio-blk devices and hit these asserts which are good to fix.

> 
> So I think we either need to also change the Xen code, or
> we need to fix this at the virtio level by having it keep
> track of the buffer it was handed so it can unmap it.
Maybe a fix at virtio level will be better in general, what do you
think?

Thanks, Dima.

> 
> thanks
> -- PMM