diff mbox series

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

Message ID 20200903161446.29615-6-eperezma@redhat.com
State Accepted
Commit 1804857f19f612f6907832e35599cdb51d4ec764
Headers show
Series [RFC,1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier | expand

Commit Message

Eugenio Perez Martin Sept. 3, 2020, 4:14 p.m. UTC 
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(-)

Comments

Eugenio Perez Martin Sept. 28, 2020, 9:05 a.m. UTC | #1 
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);

> >       }

> >   }

> >

>
Peter Xu Sept. 28, 2020, 5:48 p.m. UTC | #2 
On Mon, Sep 28, 2020 at 11:05:01AM +0200, Eugenio Perez Martin wrote:
> 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?


I think Jason is right - since we have checked at the entry that the two
regions cross over each other:

    /*
     * Skip the notification if the notification does not overlap
     * with registered range.
     */
    if (notifier->start > entry_end || notifier->end < entry->iova) {
        return;
    }

Then I don't see how this assertion can fail any more.

But imho not a big problem either, and it shouldn't hurt to even keep the
assertion of above isn't that straightforward.

> 

> Is there anything else needed to pull this patch?


I didn't post a pull for this only because I shouldn't :) - the plan was that
all vt-d patches will still go via Michael's tree, iiuc.  Though at least to me
I think this series is acceptable for merging.

Though it would always be good too if Jason would still like to review it.

Jason, what's your opinion?

Thanks,

-- 
Peter Xu
Michael S. Tsirkin Oct. 3, 2020, 5:38 p.m. UTC | #3 
On Mon, Sep 28, 2020 at 01:48:57PM -0400, Peter Xu wrote:
> On Mon, Sep 28, 2020 at 11:05:01AM +0200, Eugenio Perez Martin wrote:
> > 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?
> 
> I think Jason is right - since we have checked at the entry that the two
> regions cross over each other:
> 
>     /*
>      * Skip the notification if the notification does not overlap
>      * with registered range.
>      */
>     if (notifier->start > entry_end || notifier->end < entry->iova) {
>         return;
>     }
> 
> Then I don't see how this assertion can fail any more.
> 
> But imho not a big problem either, and it shouldn't hurt to even keep the
> assertion of above isn't that straightforward.
> 
> > 
> > Is there anything else needed to pull this patch?
> 
> I didn't post a pull for this only because I shouldn't :) - the plan was that
> all vt-d patches will still go via Michael's tree, iiuc.  Though at least to me
> I think this series is acceptable for merging.

Sure, that's ok.

Eugenio, you sent patch 0 as a response to another series, which
made me miss the series. Pls don't do that in the future.

Looks like Jason reviewed the series - Jason, is that right? -
so I'd like his ack if possible.


> Though it would always be good too if Jason would still like to review it.
> 
> Jason, what's your opinion?
> 
> Thanks,
> 
> -- 
> Peter Xu
Eugenio Perez Martin Oct. 5, 2020, 6:32 a.m. UTC | #4 
On Sat, Oct 3, 2020 at 7:38 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>

> On Mon, Sep 28, 2020 at 01:48:57PM -0400, Peter Xu wrote:

> > On Mon, Sep 28, 2020 at 11:05:01AM +0200, Eugenio Perez Martin wrote:

> > > 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?

> >

> > I think Jason is right - since we have checked at the entry that the two

> > regions cross over each other:

> >

> >     /*

> >      * Skip the notification if the notification does not overlap

> >      * with registered range.

> >      */

> >     if (notifier->start > entry_end || notifier->end < entry->iova) {

> >         return;

> >     }

> >

> > Then I don't see how this assertion can fail any more.

> >

> > But imho not a big problem either, and it shouldn't hurt to even keep the

> > assertion of above isn't that straightforward.

> >

> > >

> > > Is there anything else needed to pull this patch?

> >

> > I didn't post a pull for this only because I shouldn't :) - the plan was that

> > all vt-d patches will still go via Michael's tree, iiuc.  Though at least to me

> > I think this series is acceptable for merging.

>

> Sure, that's ok.

>

> Eugenio, you sent patch 0 as a response to another series, which

> made me miss the series. Pls don't do that in the future.

>


Sorry, noted for the next time.

Thanks!

> Looks like Jason reviewed the series - Jason, is that right? -

> so I'd like his ack if possible.

>

>

> > Though it would always be good too if Jason would still like to review it.

> >

> > Jason, what's your opinion?

> >

> > Thanks,

> >

> > --

> > Peter Xu

>
Jason Wang Oct. 15, 2020, 7:50 a.m. UTC | #5 
On 2020/10/4 上午1:38, Michael S. Tsirkin wrote:
> On Mon, Sep 28, 2020 at 01:48:57PM -0400, Peter Xu wrote:

>> On Mon, Sep 28, 2020 at 11:05:01AM +0200, Eugenio Perez Martin wrote:

>>> 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?

>> I think Jason is right - since we have checked at the entry that the two

>> regions cross over each other:

>>

>>      /*

>>       * Skip the notification if the notification does not overlap

>>       * with registered range.

>>       */

>>      if (notifier->start > entry_end || notifier->end < entry->iova) {

>>          return;

>>      }

>>

>> Then I don't see how this assertion can fail any more.

>>

>> But imho not a big problem either, and it shouldn't hurt to even keep the

>> assertion of above isn't that straightforward.

>>

>>> Is there anything else needed to pull this patch?

>> I didn't post a pull for this only because I shouldn't :) - the plan was that

>> all vt-d patches will still go via Michael's tree, iiuc.  Though at least to me

>> I think this series is acceptable for merging.

> Sure, that's ok.

>

> Eugenio, you sent patch 0 as a response to another series, which

> made me miss the series. Pls don't do that in the future.

>

> Looks like Jason reviewed the series - Jason, is that right? -

> so I'd like his ack if possible.



Right.

Euenio. If it's possible I would prefer to remove the assertion but it's 
ok it you leave it.

And please repost the series.

Thanks


>

>

>> Though it would always be good too if Jason would still like to review it.

>>

>> Jason, what's your opinion?

>>

>> Thanks,

>>

>> -- 

>> Peter Xu

>
diff mbox series

Patch

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