On Tue, 2016-01-05 at 17:03 +0100, Pierre Morel wrote:
> In vfio_listener_region_add(), the code makes sure
> that the offset in the section is lower than the size
> of the section.
> But the calculation uses size of the region instead of
> the region's limit (size - 1).
We're really just trying to validate that the region is not zero sized
and hasn't overflowed the addresses space.
> 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.
>
> Signed-off-by: Pierre Morel <pmo...@linux.vnet.ibm.com>
> ---
> hw/vfio/common.c | 15 ++++++++++-----
> 1 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 6797208..fe4962a 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -342,18 +342,23 @@ static void vfio_listener_region_add(MemoryListener
> *listener,
>
> iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
> llend = int128_make64(section->offset_within_address_space);
> - llend = int128_add(llend, section->size);
> +
> + if (int128_ge(llend, int128_2_64())) {
We've just set llend using int128_make64, so this is guaranteed false.
> + llend = int128_add(llend, int128_sub(section->size, int128_one()));
> + } else {
> + llend = int128_add(llend, section->size);
> + }
So the above changed nothing.
> llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
>
> - if (int128_ge(int128_make64(iova), llend)) {
> + if (int128_gt(int128_make64(iova), llend)) {
And this allows zero sized regions through.
> return;
> }
> end = int128_get64(llend);
>
> - 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);
This looks wrong too, max_iova is set to the last valid iova, for
instance if the iommu only supported a 4k address space, max_iova would
be 0xfff. A mapping of size 4k at offset 0 should work, but this
change would cause it to fail.
> ret = -EFAULT;
> goto fail;
> }
> @@ -363,7 +368,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
I think maybe you want to set end using:
end = int128_get64(int128_sub(llend, int128_one()));
Then removing the -1 in other places becomes correct, BUT we need to
add 1 where we're passing the size, end - iova - > end - iova + 1.
Thanks,
Alex
