Re: [PATCH v2 1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

[Alex Williamson]

On Tue, 6 Dec 2016 23:56:54 +0530
Kirti Wankhede  wrote:

> Passing zero for the size to vfio_find_dma() isn't compatible with
> matching the start address of an existing vfio_dma. Doing so triggers a
> corner case. In vfio_find_dma(), when the start address is equal to
> dma->iova and size is 0, check for the end of search range makes it to
> take wrong side of RB-tree. That fails the search even though the address
> is present in mapped dma ranges. Due to this, in vfio_dma_do_unmap(),
> while checking boundary conditions, size should be set to 1 for verifying
> start address of unmap range.
> vfio_find_dma() is also used to verify last address in unmap range with
> size = 0, but in that case address to be searched is calculated with
> size - 1 and so it works correctly.

I think this last sentence should actually read "start + size - 1", or
at least that makes it more understandable for me.  I'll make that
change and apply these. Thanks,

Alex

> Signed-off-by: Kirti Wankhede 
> Signed-off-by: Neo Jia 
> ---
>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index a28fbddb505c..8e9e94ccb2ff 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -826,7 +826,7 @@ again:
>* mappings within the range.
>*/
>   if (iommu->v2) {
> - dma = vfio_find_dma(iommu, unmap->iova, 0);
> + dma = vfio_find_dma(iommu, unmap->iova, 1);
>   if (dma && dma->iova != unmap->iova) {
>   ret = -EINVAL;
>   goto unlock;

Re: [PATCH v2 1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

[Alex Williamson]

On Tue, 6 Dec 2016 23:56:54 +0530
Kirti Wankhede  wrote:

> Passing zero for the size to vfio_find_dma() isn't compatible with
> matching the start address of an existing vfio_dma. Doing so triggers a
> corner case. In vfio_find_dma(), when the start address is equal to
> dma->iova and size is 0, check for the end of search range makes it to
> take wrong side of RB-tree. That fails the search even though the address
> is present in mapped dma ranges. Due to this, in vfio_dma_do_unmap(),
> while checking boundary conditions, size should be set to 1 for verifying
> start address of unmap range.
> vfio_find_dma() is also used to verify last address in unmap range with
> size = 0, but in that case address to be searched is calculated with
> size - 1 and so it works correctly.

I think this last sentence should actually read "start + size - 1", or
at least that makes it more understandable for me.  I'll make that
change and apply these. Thanks,

Alex

> Signed-off-by: Kirti Wankhede 
> Signed-off-by: Neo Jia 
> ---
>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index a28fbddb505c..8e9e94ccb2ff 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -826,7 +826,7 @@ again:
>* mappings within the range.
>*/
>   if (iommu->v2) {
> - dma = vfio_find_dma(iommu, unmap->iova, 0);
> + dma = vfio_find_dma(iommu, unmap->iova, 1);
>   if (dma && dma->iova != unmap->iova) {
>   ret = -EINVAL;
>   goto unlock;

[PATCH v2 1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

[Kirti Wankhede]

Passing zero for the size to vfio_find_dma() isn't compatible with
matching the start address of an existing vfio_dma. Doing so triggers a
corner case. In vfio_find_dma(), when the start address is equal to
dma->iova and size is 0, check for the end of search range makes it to
take wrong side of RB-tree. That fails the search even though the address
is present in mapped dma ranges. Due to this, in vfio_dma_do_unmap(),
while checking boundary conditions, size should be set to 1 for verifying
start address of unmap range.
vfio_find_dma() is also used to verify last address in unmap range with
size = 0, but in that case address to be searched is calculated with
size - 1 and so it works correctly.

Signed-off-by: Kirti Wankhede 
Signed-off-by: Neo Jia 
---
 drivers/vfio/vfio_iommu_type1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a28fbddb505c..8e9e94ccb2ff 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -826,7 +826,7 @@ again:
 * mappings within the range.
 */
if (iommu->v2) {
-   dma = vfio_find_dma(iommu, unmap->iova, 0);
+   dma = vfio_find_dma(iommu, unmap->iova, 1);
if (dma && dma->iova != unmap->iova) {
ret = -EINVAL;
goto unlock;
-- 
2.7.0

[PATCH v2 1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

[Kirti Wankhede]

Passing zero for the size to vfio_find_dma() isn't compatible with
matching the start address of an existing vfio_dma. Doing so triggers a
corner case. In vfio_find_dma(), when the start address is equal to
dma->iova and size is 0, check for the end of search range makes it to
take wrong side of RB-tree. That fails the search even though the address
is present in mapped dma ranges. Due to this, in vfio_dma_do_unmap(),
while checking boundary conditions, size should be set to 1 for verifying
start address of unmap range.
vfio_find_dma() is also used to verify last address in unmap range with
size = 0, but in that case address to be searched is calculated with
size - 1 and so it works correctly.

Signed-off-by: Kirti Wankhede 
Signed-off-by: Neo Jia 
---
 drivers/vfio/vfio_iommu_type1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a28fbddb505c..8e9e94ccb2ff 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -826,7 +826,7 @@ again:
 * mappings within the range.
 */
if (iommu->v2) {
-   dma = vfio_find_dma(iommu, unmap->iova, 0);
+   dma = vfio_find_dma(iommu, unmap->iova, 1);
if (dma && dma->iova != unmap->iova) {
ret = -EINVAL;
goto unlock;
-- 
2.7.0

Re: [PATCH v2 1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

[Kirti Wankhede]

On 12/6/2016 11:08 PM, Alex Williamson wrote:
> On Tue, 6 Dec 2016 22:43:30 +0530
> Kirti Wankhede  wrote:
> 
>> vfio_dma keeps track of address range from (dma->iova + 0) to
>> (dma->iova + dma->size - 1), while vfio_find_dma() search logic looks for
>> range from (dma->iova + 1) to (dma->iova + dma->size).
> 
> This is not true.  The issue is with the non-inclusive address at the
> end of a range being incompatible with passing zero for the range
> size.  Passing zero for the range size is a bit of a hack and doing
> such triggers a corner case in the end of range detection where we test
> (start + size <= dma->iova).  Using <= here covers the non-inclusive
> range end, ie. if the range was start=0x0, size=0x1000, the range is
> actually 0x0-0xfff.  Thus if start+size is 0x1000, it should not be
> considered part of a range beginning at 0x1000.  So there's no
> incompatibility as implied in the statement above, it's more that using
> zero for the size isn't compatible with matching the start address of
> an existing vfio_dma.  Thanks,
> 

Makes sense. Updating the description of both patch.

Thanks,
Kirti

> Alex
> 
>> In vfio_find_dma(), when the start address is equal to dma->iova and size
>> is 0, check for the end of search range makes it to take wrong side of
>> RB-tree. That fails the search even though the address is present in
>> mapped dma ranges. Due to this, in vfio_dma_do_unmap(), while checking
>> boundary conditions, size should be set to 1 for verifying start address
>> of unmap range.
>> vfio_find_dma() is also used to verify last address in unmap range with
>> size = 0, but in that case address to be searched is calculated with
>> size - 1 and so it works correctly.
>>
>> Signed-off-by: Kirti Wankhede 
>> Signed-off-by: Neo Jia 
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c 
>> b/drivers/vfio/vfio_iommu_type1.c
>> index a28fbddb505c..8e9e94ccb2ff 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -826,7 +826,7 @@ again:
>>   * mappings within the range.
>>   */
>>  if (iommu->v2) {
>> -dma = vfio_find_dma(iommu, unmap->iova, 0);
>> +dma = vfio_find_dma(iommu, unmap->iova, 1);
>>  if (dma && dma->iova != unmap->iova) {
>>  ret = -EINVAL;
>>  goto unlock;
> 

Re: [PATCH v2 1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

[Kirti Wankhede]

On 12/6/2016 11:08 PM, Alex Williamson wrote:
> On Tue, 6 Dec 2016 22:43:30 +0530
> Kirti Wankhede  wrote:
> 
>> vfio_dma keeps track of address range from (dma->iova + 0) to
>> (dma->iova + dma->size - 1), while vfio_find_dma() search logic looks for
>> range from (dma->iova + 1) to (dma->iova + dma->size).
> 
> This is not true.  The issue is with the non-inclusive address at the
> end of a range being incompatible with passing zero for the range
> size.  Passing zero for the range size is a bit of a hack and doing
> such triggers a corner case in the end of range detection where we test
> (start + size <= dma->iova).  Using <= here covers the non-inclusive
> range end, ie. if the range was start=0x0, size=0x1000, the range is
> actually 0x0-0xfff.  Thus if start+size is 0x1000, it should not be
> considered part of a range beginning at 0x1000.  So there's no
> incompatibility as implied in the statement above, it's more that using
> zero for the size isn't compatible with matching the start address of
> an existing vfio_dma.  Thanks,
> 

Makes sense. Updating the description of both patch.

Thanks,
Kirti

> Alex
> 
>> In vfio_find_dma(), when the start address is equal to dma->iova and size
>> is 0, check for the end of search range makes it to take wrong side of
>> RB-tree. That fails the search even though the address is present in
>> mapped dma ranges. Due to this, in vfio_dma_do_unmap(), while checking
>> boundary conditions, size should be set to 1 for verifying start address
>> of unmap range.
>> vfio_find_dma() is also used to verify last address in unmap range with
>> size = 0, but in that case address to be searched is calculated with
>> size - 1 and so it works correctly.
>>
>> Signed-off-by: Kirti Wankhede 
>> Signed-off-by: Neo Jia 
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c 
>> b/drivers/vfio/vfio_iommu_type1.c
>> index a28fbddb505c..8e9e94ccb2ff 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -826,7 +826,7 @@ again:
>>   * mappings within the range.
>>   */
>>  if (iommu->v2) {
>> -dma = vfio_find_dma(iommu, unmap->iova, 0);
>> +dma = vfio_find_dma(iommu, unmap->iova, 1);
>>  if (dma && dma->iova != unmap->iova) {
>>  ret = -EINVAL;
>>  goto unlock;
> 

Re: [PATCH v2 1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

[Alex Williamson]

On Tue, 6 Dec 2016 22:43:30 +0530
Kirti Wankhede  wrote:

> vfio_dma keeps track of address range from (dma->iova + 0) to
> (dma->iova + dma->size - 1), while vfio_find_dma() search logic looks for
> range from (dma->iova + 1) to (dma->iova + dma->size).

This is not true.  The issue is with the non-inclusive address at the
end of a range being incompatible with passing zero for the range
size.  Passing zero for the range size is a bit of a hack and doing
such triggers a corner case in the end of range detection where we test
(start + size <= dma->iova).  Using <= here covers the non-inclusive
range end, ie. if the range was start=0x0, size=0x1000, the range is
actually 0x0-0xfff.  Thus if start+size is 0x1000, it should not be
considered part of a range beginning at 0x1000.  So there's no
incompatibility as implied in the statement above, it's more that using
zero for the size isn't compatible with matching the start address of
an existing vfio_dma.  Thanks,

Alex

> In vfio_find_dma(), when the start address is equal to dma->iova and size
> is 0, check for the end of search range makes it to take wrong side of
> RB-tree. That fails the search even though the address is present in
> mapped dma ranges. Due to this, in vfio_dma_do_unmap(), while checking
> boundary conditions, size should be set to 1 for verifying start address
> of unmap range.
> vfio_find_dma() is also used to verify last address in unmap range with
> size = 0, but in that case address to be searched is calculated with
> size - 1 and so it works correctly.
> 
> Signed-off-by: Kirti Wankhede 
> Signed-off-by: Neo Jia 
> ---
>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index a28fbddb505c..8e9e94ccb2ff 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -826,7 +826,7 @@ again:
>* mappings within the range.
>*/
>   if (iommu->v2) {
> - dma = vfio_find_dma(iommu, unmap->iova, 0);
> + dma = vfio_find_dma(iommu, unmap->iova, 1);
>   if (dma && dma->iova != unmap->iova) {
>   ret = -EINVAL;
>   goto unlock;

Re: [PATCH v2 1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

[Alex Williamson]

On Tue, 6 Dec 2016 22:43:30 +0530
Kirti Wankhede  wrote:

> vfio_dma keeps track of address range from (dma->iova + 0) to
> (dma->iova + dma->size - 1), while vfio_find_dma() search logic looks for
> range from (dma->iova + 1) to (dma->iova + dma->size).

This is not true.  The issue is with the non-inclusive address at the
end of a range being incompatible with passing zero for the range
size.  Passing zero for the range size is a bit of a hack and doing
such triggers a corner case in the end of range detection where we test
(start + size <= dma->iova).  Using <= here covers the non-inclusive
range end, ie. if the range was start=0x0, size=0x1000, the range is
actually 0x0-0xfff.  Thus if start+size is 0x1000, it should not be
considered part of a range beginning at 0x1000.  So there's no
incompatibility as implied in the statement above, it's more that using
zero for the size isn't compatible with matching the start address of
an existing vfio_dma.  Thanks,

Alex

> In vfio_find_dma(), when the start address is equal to dma->iova and size
> is 0, check for the end of search range makes it to take wrong side of
> RB-tree. That fails the search even though the address is present in
> mapped dma ranges. Due to this, in vfio_dma_do_unmap(), while checking
> boundary conditions, size should be set to 1 for verifying start address
> of unmap range.
> vfio_find_dma() is also used to verify last address in unmap range with
> size = 0, but in that case address to be searched is calculated with
> size - 1 and so it works correctly.
> 
> Signed-off-by: Kirti Wankhede 
> Signed-off-by: Neo Jia 
> ---
>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index a28fbddb505c..8e9e94ccb2ff 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -826,7 +826,7 @@ again:
>* mappings within the range.
>*/
>   if (iommu->v2) {
> - dma = vfio_find_dma(iommu, unmap->iova, 0);
> + dma = vfio_find_dma(iommu, unmap->iova, 1);
>   if (dma && dma->iova != unmap->iova) {
>   ret = -EINVAL;
>   goto unlock;

[PATCH v2 1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

[Kirti Wankhede]

vfio_dma keeps track of address range from (dma->iova + 0) to
(dma->iova + dma->size - 1), while vfio_find_dma() search logic looks for
range from (dma->iova + 1) to (dma->iova + dma->size).
In vfio_find_dma(), when the start address is equal to dma->iova and size
is 0, check for the end of search range makes it to take wrong side of
RB-tree. That fails the search even though the address is present in
mapped dma ranges. Due to this, in vfio_dma_do_unmap(), while checking
boundary conditions, size should be set to 1 for verifying start address
of unmap range.
vfio_find_dma() is also used to verify last address in unmap range with
size = 0, but in that case address to be searched is calculated with
size - 1 and so it works correctly.

Signed-off-by: Kirti Wankhede 
Signed-off-by: Neo Jia 
---
 drivers/vfio/vfio_iommu_type1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a28fbddb505c..8e9e94ccb2ff 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -826,7 +826,7 @@ again:
 * mappings within the range.
 */
if (iommu->v2) {
-   dma = vfio_find_dma(iommu, unmap->iova, 0);
+   dma = vfio_find_dma(iommu, unmap->iova, 1);
if (dma && dma->iova != unmap->iova) {
ret = -EINVAL;
goto unlock;
-- 
2.7.0

[PATCH v2 1/2] vfio iommu type1: Fix size argument to vfio_find_dma() during DMA UNMAP.

[Kirti Wankhede]

vfio_dma keeps track of address range from (dma->iova + 0) to
(dma->iova + dma->size - 1), while vfio_find_dma() search logic looks for
range from (dma->iova + 1) to (dma->iova + dma->size).
In vfio_find_dma(), when the start address is equal to dma->iova and size
is 0, check for the end of search range makes it to take wrong side of
RB-tree. That fails the search even though the address is present in
mapped dma ranges. Due to this, in vfio_dma_do_unmap(), while checking
boundary conditions, size should be set to 1 for verifying start address
of unmap range.
vfio_find_dma() is also used to verify last address in unmap range with
size = 0, but in that case address to be searched is calculated with
size - 1 and so it works correctly.

Signed-off-by: Kirti Wankhede 
Signed-off-by: Neo Jia 
---
 drivers/vfio/vfio_iommu_type1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a28fbddb505c..8e9e94ccb2ff 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -826,7 +826,7 @@ again:
 * mappings within the range.
 */
if (iommu->v2) {
-   dma = vfio_find_dma(iommu, unmap->iova, 0);
+   dma = vfio_find_dma(iommu, unmap->iova, 1);
if (dma && dma->iova != unmap->iova) {
ret = -EINVAL;
goto unlock;
-- 
2.7.0