vfio: add check for memory region overflow condition

vfio_listener_region_add for a iommu mr results in
an overflow assert since emulated iommu memory region is initialized with UINT64_MAX. Add a check just like memory_region_size() does.

Signed-off-by Bandan Das <[email protected]>

---
 hw/vfio/common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

hw/vfio/common.c +0 -0

index fb588d8..269244b 100644
@@ -349,7 +349,12 @@ static void vfio_listener_region_add(MemoryListener *listener,
     if (int128_ge(int128_make64(iova), llend)) {
         return;
     }

Quoted 26 lines · On Mon, 21 Mar 2016 18:00:50 -0400 Bandan Das <[email protected]> wrote:

vfio_listener_region_add for a iommu mr results in
an overflow assert since emulated iommu memory region is initialized with UINT64_MAX. Add a check just like memory_region_size() does.

Signed-off-by Bandan Das <[email protected]>

---
 hw/vfio/common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

hw/vfio/common.c +6 -1

index fb588d8..269244b 100644
@@ -349,7 +349,12 @@ static void vfio_listener_region_add(MemoryListener *listener,
     if (int128_ge(int128_make64(iova), llend)) {
         return;
     }
-    end = int128_get64(llend);
+
+    if (int128_eq(llend, int128_2_64())) {
+            end = UINT64_MAX;
+    } else {
+            end = int128_get64(llend);
+    }
 

hw/vfio/common.c static void vfio_listener_region_add(MemoryListener *listener, · line 360

Quoted 2 lines

hw/vfio/common.c +0 -0

     if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
         error_report("vfio: IOMMU container %p can't map guest IOVA region"

But now all the calculations where we use end-1 are wrong. See the discussion with Pierre Morel in the January qemu-devel archives. There's a solution in there, but I never saw a follow-up from Pierre with a revised patch. Thanks,

Alex

Quoted 29 lines · Alex Williamson

Quoted 26 lines · On Mon, 21 Mar 2016 18:00:50 -0400 Bandan Das <[email protected]> wrote:

vfio_listener_region_add for a iommu mr results in
an overflow assert since emulated iommu memory region is initialized with UINT64_MAX. Add a check just like memory_region_size() does.

Signed-off-by Bandan Das <[email protected]>

---
 hw/vfio/common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

hw/vfio/common.c +6 -1

index fb588d8..269244b 100644
@@ -349,7 +349,12 @@ static void vfio_listener_region_add(MemoryListener *listener,
     if (int128_ge(int128_make64(iova), llend)) {
         return;
     }
-    end = int128_get64(llend);
+
+    if (int128_eq(llend, int128_2_64())) {
+            end = UINT64_MAX;
+    } else {
+            end = int128_get64(llend);
+    }
 

Quoted 7 lines

hw/vfio/common.c static void vfio_listener_region_add(MemoryListener *listener, · line 360

Quoted 2 lines

hw/vfio/common.c +0 -0

     if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
         error_report("vfio: IOMMU container %p can't map guest IOVA region"  

But now all the calculations where we use end-1 are wrong. See the discussion with Pierre Morel in the January qemu-devel archives. There's a solution in there, but I never saw a follow-up from Pierre with a revised patch. Thanks,

I am missing something. When end < UIN64_MAX, end - 1 calculations are valid because the patch doesn't change that behavior. When end is UINT64_MAX, int128_get64() doesn't know how to calculate this value and we are just feeding it manually. The patch is just the opposite of what memory_region_init() did to init the mem region in the first place:

   mr->size = int128_make64(size);
   if (size == UINT64_MAX) {
      mr->size = int128_2_64();
   }

So, end - 1 is still valid for end = UINT64_MAX, no ?

Quoted 43 lines · On Mon, 21 Mar 2016 20:06:32 -0400 Bandan Das <[email protected]> wrote:

Quoted 29 lines · Alex Williamson

Quoted 26 lines · On Mon, 21 Mar 2016 18:00:50 -0400 Bandan Das <[email protected]> wrote:

vfio_listener_region_add for a iommu mr results in
an overflow assert since emulated iommu memory region is initialized with UINT64_MAX. Add a check just like memory_region_size() does.

Signed-off-by Bandan Das <[email protected]>

---
 hw/vfio/common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

hw/vfio/common.c +6 -1

index fb588d8..269244b 100644
@@ -349,7 +349,12 @@ static void vfio_listener_region_add(MemoryListener *listener,
     if (int128_ge(int128_make64(iova), llend)) {
         return;
     }
-    end = int128_get64(llend);
+
+    if (int128_eq(llend, int128_2_64())) {
+            end = UINT64_MAX;
+    } else {
+            end = int128_get64(llend);
+    }
 

Quoted 7 lines

hw/vfio/common.c static void vfio_listener_region_add(MemoryListener *listener, · line 360

Quoted 2 lines

hw/vfio/common.c +0 -0

     if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
         error_report("vfio: IOMMU container %p can't map guest IOVA region"  

But now all the calculations where we use end-1 are wrong. See the discussion with Pierre Morel in the January qemu-devel archives. There's a solution in there, but I never saw a follow-up from Pierre with a revised patch. Thanks,

I am missing something. When end < UIN64_MAX, end - 1 calculations are valid because the patch doesn't change that behavior. When end is UINT64_MAX, int128_get64() doesn't know how to calculate this value and we are just feeding it manually. The patch is just the opposite of what memory_region_init() did to init the mem region in the first place:

Quoted 5 lines

   mr->size = int128_make64(size);
   if (size == UINT64_MAX) {
      mr->size = int128_2_64();
   }

So, end - 1 is still valid for end = UINT64_MAX, no ?

int128_2_64() is not equal to UINT64_MAX, so assigning UIN64_MAX to @end is clearing altering the value. If we had a range from zero to

int128_2_64() then the size of that region is int128_2_64(). If we alter @end to be UINT64_MAX, then the size is only UINT64_MAX and @end
- 1 is off by one versus the case where we use the value directly.

You're effectively changing @end to be the last address in the range,

but only in some cases, and not adjusting the remaining code to match. Not only that, but the vfio map command is probably going to fail if we pass in such an unaligned size since the mapping granularity is

likely the system page size. Thanks,

Alex

Quoted 52 lines · Alex Williamson

Quoted 48 lines · On Mon, 21 Mar 2016 20:06:32 -0400 Bandan Das <[email protected]> wrote:

Quoted 29 lines · Alex Williamson

Quoted 26 lines · On Mon, 21 Mar 2016 18:00:50 -0400 Bandan Das <[email protected]> wrote:

vfio_listener_region_add for a iommu mr results in
an overflow assert since emulated iommu memory region is initialized with UINT64_MAX. Add a check just like memory_region_size() does.

Signed-off-by Bandan Das <[email protected]>

---
 hw/vfio/common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

hw/vfio/common.c +6 -1

index fb588d8..269244b 100644
@@ -349,7 +349,12 @@ static void vfio_listener_region_add(MemoryListener *listener,
     if (int128_ge(int128_make64(iova), llend)) {
         return;
     }
-    end = int128_get64(llend);
+
+    if (int128_eq(llend, int128_2_64())) {
+            end = UINT64_MAX;
+    } else {
+            end = int128_get64(llend);
+    }
 

Quoted 7 lines

hw/vfio/common.c static void vfio_listener_region_add(MemoryListener *listener, · line 360

Quoted 2 lines

hw/vfio/common.c +0 -0

     if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
         error_report("vfio: IOMMU container %p can't map guest IOVA region"    

But now all the calculations where we use end-1 are wrong. See the discussion with Pierre Morel in the January qemu-devel archives. There's a solution in there, but I never saw a follow-up from Pierre with a revised patch. Thanks,

I am missing something. When end < UIN64_MAX, end - 1 calculations are valid because the patch doesn't change that behavior. When end is UINT64_MAX, int128_get64() doesn't know how to calculate this value and we are just feeding it manually. The patch is just the opposite of what memory_region_init() did to init the mem region in the first place:

   mr->size = int128_make64(size);
   if (size == UINT64_MAX) {
      mr->size = int128_2_64();
   }

So, end - 1 is still valid for end = UINT64_MAX, no ?

Quoted 2 lines

int128_2_64() is not equal to UINT64_MAX, so assigning UIN64_MAX to @end is clearing altering the value. If we had a range from zero to

I thought in128_2_64 is the 128 bit representation of UINT64_MAX. The if condition in memory_region_init doesn't make sense otherwise.

Quoted 3 lines

int128_2_64() then the size of that region is int128_2_64(). If we alter @end to be UINT64_MAX, then the size is only UINT64_MAX and @end
- 1 is off by one versus the case where we use the value directly.

Ok, you mean something like:
int128_get64(int128_sub(int128_2_64(), int128_make64(1))); for (end - 1) ? But we still have to deal with (end - iova) when calling vfio_dmap_map(). int128_get64() will definitely assert for iova = 0.

Quoted 1 line

You're effectively changing @end to be the last address in the range,

No, I think I am changing "end" to what we initally started with for size before converting to 128 bit.

Quoted 3 lines

but only in some cases, and not adjusting the remaining code to match. Not only that, but the vfio map command is probably going to fail if we pass in such an unaligned size since the mapping granularity is

Trying to map such a large region is wrong anyway, I am still trying to workout a solution to avoid calling memory_region_init_iommu() with UINT64_MAX which is what emulated vt-d currently does.

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

   mr->size = int128_make64(size);
   if (size == UINT64_MAX) {
      mr->size = int128_2_64();
   }

So, end - 1 is still valid for end = UINT64_MAX, no ?

int128_2_64() is not equal to UINT64_MAX, so assigning UIN64_MAX to @end is clearing altering the value. If we had a range from zero to

Quoted 2 lines

I thought in128_2_64 is the 128 bit representation of UINT64_MAX. The if condition in memory_region_init doesn't make sense otherwise.

2^64 cannot be represented with a uint64_t, 2^64 - 1 can:

int128_2_64 = 1_0000_0000_0000_0000h
UINT64_MAX = ffff_ffff_ffff_ffffh

Quoted 8 lines

Quoted 3 lines

int128_2_64() then the size of that region is int128_2_64(). If we alter @end to be UINT64_MAX, then the size is only UINT64_MAX and @end
- 1 is off by one versus the case where we use the value directly.

Ok, you mean something like:
int128_get64(int128_sub(int128_2_64(), int128_make64(1))); for (end - 1) ? But we still have to deal with (end - iova) when calling vfio_dmap_map(). int128_get64() will definitely assert for iova = 0.

I don't know that that's the most efficient way to handle it, but @end represents a different thing by imposing that -1 and it needs to be handled in the reset of the code.

Quoted 4 lines

Quoted 1 line

You're effectively changing @end to be the last address in the range,

No, I think I am changing "end" to what we initally started with for size before converting to 128 bit.

Nope, it's the difference between the size of the region and the last address of the region.

Quoted 7 lines

Quoted 3 lines

but only in some cases, and not adjusting the remaining code to match. Not only that, but the vfio map command is probably going to fail if we pass in such an unaligned size since the mapping granularity is

Trying to map such a large region is wrong anyway, I am still trying to workout a solution to avoid calling memory_region_init_iommu() with UINT64_MAX which is what emulated vt-d currently does.

Right, the address width of the IOMMU on x86 is typically nowhere near 2^64, so if you take the vfio_dma_map path, you'll surely explode.

Does this fix actually fix anything or just move us to the next assert? Thanks,

Alex

Alex Williamson <[email protected]> writes:
...

Quoted 14 lines

Quoted 11 lines

Quoted 8 lines

Quoted 5 lines

   mr->size = int128_make64(size);
   if (size == UINT64_MAX) {
      mr->size = int128_2_64();
   }

So, end - 1 is still valid for end = UINT64_MAX, no ?

int128_2_64() is not equal to UINT64_MAX, so assigning UIN64_MAX to @end is clearing altering the value. If we had a range from zero to

I thought in128_2_64 is the 128 bit representation of UINT64_MAX. The if condition in memory_region_init doesn't make sense otherwise.

2^64 cannot be represented with a uint64_t, 2^64 - 1 can:

Quoted 2 lines

int128_2_64 = 1_0000_0000_0000_0000h
UINT64_MAX = ffff_ffff_ffff_ffffh

Thanks, understood this part. I still don't understand the if condition in memory_region_init however. Unless, that function actually takes the last address for the size parameter and in that case, it should be UINT64_MAX-1 for a size of UINT64_MAX.

Quoted 18 lines

Quoted 8 lines

Quoted 3 lines

int128_2_64() then the size of that region is int128_2_64(). If we alter @end to be UINT64_MAX, then the size is only UINT64_MAX and @end
- 1 is off by one versus the case where we use the value directly.

Ok, you mean something like:
int128_get64(int128_sub(int128_2_64(), int128_make64(1))); for (end - 1) ? But we still have to deal with (end - iova) when calling vfio_dmap_map(). int128_get64() will definitely assert for iova = 0.

I don't know that that's the most efficient way to handle it, but @end represents a different thing by imposing that -1 and it needs to be handled in the reset of the code.

Quoted 4 lines

Quoted 1 line

You're effectively changing @end to be the last address in the range,

No, I think I am changing "end" to what we initally started with for size before converting to 128 bit.

Quoted 2 lines

Nope, it's the difference between the size of the region and the last address of the region.

Ok, but note that it's the "size" that actually asserts here since the offset is 0. So, we started with a size UINT64_MAX but end with mr->size = 128_2_64().

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

but only in some cases, and not adjusting the remaining code to match. Not only that, but the vfio map command is probably going to fail if we pass in such an unaligned size since the mapping granularity is

Trying to map such a large region is wrong anyway, I am still trying to workout a solution to avoid calling memory_region_init_iommu() with UINT64_MAX which is what emulated vt-d currently does.

Quoted 2 lines

Right, the address width of the IOMMU on x86 is typically nowhere near 2^64, so if you take the vfio_dma_map path, you'll surely explode.

And it does. If we fix this assert, then vfio_dma_map() attempts mapping this direct mapped address range starting from 0 and prints a warning message; happens for the whole range and goes on for ever. The overflow check seemed to me like something we should fix, but now I am more confused then ever!

Quoted 6 lines

And it does. If we fix this assert, then vfio_dma_map() attempts mapping this direct mapped address range starting from 0 and prints a warning message; happens for the whole range and goes on for ever. The overflow check seemed to me like something we should fix, but now I am more confused then ever!

Is the MemoryRegion memory_region_is_iommu() such that you're calling vfio_dma_map() from vfio_iommu_map_notify()? If so then we should

probably be using 128bit helpers for doing sanity checking and go ahead and let something assert if we get to the vfio_dma_map() in vfio_listener_region_add() with a 2^64 size. Then if you're taking the memory_region_is_iommu() path, vfio_dma_map() is going to be called with translations within that 2^64 bit address space, not mapping the entire space, right? Thanks,

Alex

vfio_listener_region_add for a iommu mr results in
an overflow assert since emulated iommu memory region is initialized with UINT64_MAX. Add a check just like memory_region_size() does.

Quoted 5 lines · Bandan Das

vfio_listener_region_add for a iommu mr results in
an overflow assert since emulated iommu memory region is initialized with UINT64_MAX. Add a check just like memory_region_size() does.

Hi, Bandan,

In case you missed this:

https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg02865.html

-- peterx

Quoted 7 lines · Peter Xu

Quoted 5 lines · Bandan Das

vfio_listener_region_add for a iommu mr results in
an overflow assert since emulated iommu memory region is initialized with UINT64_MAX. Add a check just like memory_region_size() does.

Quoted 5 lines

Hi, Bandan,

In case you missed this:

https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg02865.html

Thank you Peter, I wasn't aware. But unfortunately, I don't think he's doing the right thing either when handing @end ? Oh well, it's a RFC :)

Thank you Peter, I wasn't aware. But unfortunately, I don't think he's doing the right thing either when handing @end ? Oh well, it's a RFC :)