Pierre Morel <pmo...@linux.vnet.ibm.com> writes:
> In vfio_listener_region_add(), we try to validate that the region is not
> zero sized and hasn't overflowed the addresses space.
>
> But the calculation uses the size of the region instead of
> using the region's limit (size - 1).
>
> This leads to Int128 overflow when the region has
> been initialized to UINT64_MAX because in this case
> memory_region_init() transform the size from UINT64_MAX
> to int128_2_64().
>
> Let's really use the limit by sustracting one to the size
> and take care to use the limit for functions using limit
> and size to call functions which need size.
Can we just use the overflow condition as a special case ?
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 607ec70..ce18f3f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -349,7 +349,13 @@ 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);
+ }
and the rest of the stuff doesn't need changing.
> Signed-off-by: Pierre Morel <pmo...@linux.vnet.ibm.com>
> ---
>
> Changes from v2:
> - all, just ignore v2, sorry about this,
> this is build after v1
>
> Changes from v1:
> - adjust the tests by knowing we already substracted one to end.
>
> hw/vfio/common.c | 14 +++++++-------
> 1 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 6797208..a5f6643 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -348,12 +348,12 @@ static void vfio_listener_region_add(MemoryListener
> *listener,
> if (int128_ge(int128_make64(iova), llend)) {
> return;
> }
> - end = int128_get64(llend);
> + end = int128_get64(int128_sub(llend, int128_one()));
>
> - if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
> + if ((iova < container->min_iova) || (end > container->max_iova)) {
> error_report("vfio: IOMMU container %p can't map guest IOVA region"
> " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
> - container, iova, end - 1);
> + container, iova, end);
> ret = -EFAULT;
> goto fail;
> }
> @@ -363,7 +363,7 @@ static void vfio_listener_region_add(MemoryListener
> *listener,
> if (memory_region_is_iommu(section->mr)) {
> VFIOGuestIOMMU *giommu;
>
> - trace_vfio_listener_region_add_iommu(iova, end - 1);
> + trace_vfio_listener_region_add_iommu(iova, end);
> /*
> * FIXME: We should do some checking to see if the
> * capabilities of the host VFIO IOMMU are adequate to model
> @@ -394,13 +394,13 @@ static void vfio_listener_region_add(MemoryListener
> *listener,
> section->offset_within_region +
> (iova - section->offset_within_address_space);
>
> - trace_vfio_listener_region_add_ram(iova, end - 1, vaddr);
> + trace_vfio_listener_region_add_ram(iova, end, vaddr);
>
> - ret = vfio_dma_map(container, iova, end - iova, vaddr,
> section->readonly);
> + ret = vfio_dma_map(container, iova, end - iova + 1, vaddr,
> section->readonly);
> if (ret) {
> error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
> "0x%"HWADDR_PRIx", %p) = %d (%m)",
> - container, iova, end - iova, vaddr, ret);
> + container, iova, end - iova + 1, vaddr, ret);
> goto fail;
> }
