Re: [Qemu-devel] [PATCH v1] exec: check the range in the address_space_unmap routine

Re: [Qemu-devel] [PATCH v1] exec: check the range in the address_space_unmap routine

[Dima Stepanov]

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