Re: [PATCH 5/5] memory: Skip bad range assertion if notifier is DEVIOTLB_UNMAP type

[Eugenio Perez Martin]

On Fri, Sep 4, 2020 at 6:34 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/9/4 上午12:14, Eugenio Pérez wrote:
> > Device IOTLB invalidations can unmap arbitrary ranges, eiter outside of
> > the memory region or even [0, ~0ULL] for all the space. The assertion
> > could be hit by a guest, and rhel7 guest effectively hit it.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > ---
> >   softmmu/memory.c | 13 +++++++++++--
> >   1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 8694fc7cf7..e723fcbaa1 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -1895,6 +1895,7 @@ void memory_region_notify_iommu_one(IOMMUNotifier 
> > *notifier,
> >   {
> >       IOMMUTLBEntry *entry = &event->entry;
> >       hwaddr entry_end = entry->iova + entry->addr_mask;
> > +    IOMMUTLBEntry tmp = *entry;
> >
> >       if (event->type == IOMMU_NOTIFIER_UNMAP) {
> >           assert(entry->perm == IOMMU_NONE);
> > @@ -1908,10 +1909,18 @@ void memory_region_notify_iommu_one(IOMMUNotifier 
> > *notifier,
> >           return;
> >       }
> >
> > -    assert(entry->iova >= notifier->start && entry_end <= notifier->end);
> > +    if (notifier->notifier_flags & IOMMU_NOTIFIER_DEVIOTLB_UNMAP) {
> > +        /* Crop (iova, addr_mask) to range */
> > +        tmp.iova = MAX(tmp.iova, notifier->start);
> > +        tmp.addr_mask = MIN(entry_end, notifier->end) - tmp.iova;
> > +        /* Confirm no underflow */
> > +        assert(MIN(entry_end, notifier->end) >= tmp.iova);
>
>
> It's still not clear to me why we need such assert. Consider
> notifier->end is the possible IOVA range but not possible device IOTLB
> invalidation range (e.g it allows [0, ULLONG_MAX]).
>
> Thanks
>

As far as I understood the device should admit that out of bounds
notifications in that case,
and the assert just makes sure that there was no underflow in
tmp.addr_mask, i.e., that something
very wrong that should never happen in production happened.

Peter, would you mind to confirm/correct it?

Is there anything else needed to pull this patch?

Thanks!

>
> > +    } else {
> > +        assert(entry->iova >= notifier->start && entry_end <= 
> > notifier->end);
> > +    }
> >
> >       if (event->type & notifier->notifier_flags) {
> > -        notifier->notify(notifier, entry);
> > +        notifier->notify(notifier, &tmp);
> >       }
> >   }
> >
>