[PATCH 0/2] Coverity fixes for map_domain_page() mismatches

[PATCH 0/2] Coverity fixes for map_domain_page() mismatches

[Andrew Cooper]

In the most recent Coverity run, we submitted a modelling file which tried to
teach Coverity about map_domain_page() as allocating resource, and needing an
accompanying unmap_domain_page().

Here are two fixes directly identified by the modelling.

There were further issues identified, but I believe they are spurious and
caused by issues with the modelling itself.

There were also problems identified for tmem (which was the original cause of
trying to model this in the first place).  As I have already submitted a patch
which is also a cleanup, it can be dealt with separately.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Andrew Cooper]

Coverity ID: 1135379

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/iommu_guest.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 952600a..7687cf1 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -433,7 +433,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
 
     /* Do not update host dte before gcr3 has been set */
     if ( gcr3_gfn == 0 )
+    {
+        unmap_domain_page(dte_base);
         return 0;
+    }
 
     gcr3_mfn = mfn_x(get_gfn(d, gcr3_gfn, &p2mt));
     put_gfn(d, gcr3_gfn);
@@ -446,6 +449,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
     {
         AMD_IOMMU_DEBUG("%s: Fail to find iommu for bdf %x!\n",
                         __func__, mbdf);
+        unmap_domain_page(dte_base);
         return -ENODEV;
     }
 
-- 
1.7.10.4

[PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page()

[Andrew Cooper]

This avoids a resource leak and needless playing with the pagetables in the
case that the page is broken.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/common/page_alloc.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 8002bd2..5f484a2 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1652,11 +1652,13 @@ __initcall(pagealloc_keyhandler_init);
 
 void scrub_one_page(struct page_info *pg)
 {
-    void *p = __map_domain_page(pg);
+    void *p;
 
     if ( unlikely(pg->count_info & PGC_broken) )
         return;
 
+    p = __map_domain_page(pg);
+
 #ifndef NDEBUG
     /* Avoid callers relying on allocations returning zeroed pages. */
     memset(p, 0xc2, PAGE_SIZE);
-- 
1.7.10.4

Re: [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Jan Beulich]

>>> On 04.12.13 at 16:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -433,7 +433,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>  
>      /* Do not update host dte before gcr3 has been set */
>      if ( gcr3_gfn == 0 )
> +    {
> +        unmap_domain_page(dte_base);
>          return 0;
> +    }
>  
>      gcr3_mfn = mfn_x(get_gfn(d, gcr3_gfn, &p2mt));
>      put_gfn(d, gcr3_gfn);
> @@ -446,6 +449,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>      {
>          AMD_IOMMU_DEBUG("%s: Fail to find iommu for bdf %x!\n",
>                          __func__, mbdf);
> +        unmap_domain_page(dte_base);
>          return -ENODEV;
>      }

I think the better way to fix this would be to move

    glx = get_glx_from_dte(gdte);
    gv = get_gv_from_dte(gdte);

    unmap_domain_page(dte_base);

up ahead of the first exit path.

Jan

Re: [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page()

[Jan Beulich]

>>> On 04.12.13 at 16:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> This avoids a resource leak and needless playing with the pagetables in the
> case that the page is broken.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> CC: Tim Deegan <tim@xen.org>
> ---
>  xen/common/page_alloc.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 8002bd2..5f484a2 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1652,11 +1652,13 @@ __initcall(pagealloc_keyhandler_init);
>  
>  void scrub_one_page(struct page_info *pg)
>  {
> -    void *p = __map_domain_page(pg);
> +    void *p;
>  
>      if ( unlikely(pg->count_info & PGC_broken) )
>          return;
>  
> +    p = __map_domain_page(pg);
> +
>  #ifndef NDEBUG
>      /* Avoid callers relying on allocations returning zeroed pages. */
>      memset(p, 0xc2, PAGE_SIZE);
> -- 
> 1.7.10.4

Re: [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Andrew Cooper]

On 04/12/13 15:22, Jan Beulich wrote:
>>>> On 04.12.13 at 16:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/drivers/passthrough/amd/iommu_guest.c
>> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
>> @@ -433,7 +433,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>>  
>>      /* Do not update host dte before gcr3 has been set */
>>      if ( gcr3_gfn == 0 )
>> +    {
>> +        unmap_domain_page(dte_base);
>>          return 0;
>> +    }
>>  
>>      gcr3_mfn = mfn_x(get_gfn(d, gcr3_gfn, &p2mt));
>>      put_gfn(d, gcr3_gfn);
>> @@ -446,6 +449,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>>      {
>>          AMD_IOMMU_DEBUG("%s: Fail to find iommu for bdf %x!\n",
>>                          __func__, mbdf);
>> +        unmap_domain_page(dte_base);
>>          return -ENODEV;
>>      }
> I think the better way to fix this would be to move
>
>     glx = get_glx_from_dte(gdte);
>     gv = get_gv_from_dte(gdte);
>
>     unmap_domain_page(dte_base);
>
> up ahead of the first exit path.
>
> Jan
>

So it would - v2 on its way.

~Andrew

[PATCH v2 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Andrew Cooper]

Coverity ID: 1135379

As the code stands, the domain mapping will be leaked on each error path.

The mapping can be for a much shorter period of time, and all the relevent
information can be pulled out at once.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

---

Changes in v2:
 * Reduce the mapping period (Suggested by Jan)

---
 xen/drivers/passthrough/amd/iommu_guest.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 952600a..0e0a7bb 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -399,7 +399,7 @@ static int do_completion_wait(struct domain *d, cmd_entry_t *cmd)
 static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
 {
     uint16_t gbdf, mbdf, req_id, gdom_id, hdom_id;
-    dev_entry_t *gdte, *mdte, *dte_base;
+    dev_entry_t *gdte, *mdte;
     struct amd_iommu *iommu = NULL;
     struct guest_iommu *g_iommu;
     uint64_t gcr3_gfn, gcr3_mfn;
@@ -424,12 +424,14 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
                                         sizeof(dev_entry_t), gbdf);
     ASSERT(mfn_valid(dte_mfn));
 
-    dte_base = map_domain_page(dte_mfn);
-
-    gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t));
+    gdte = map_domain_page(dte_mfn) + gbdf % (PAGE_SIZE / sizeof(dev_entry_t));
 
     gdom_id  = get_domid_from_dte(gdte);
     gcr3_gfn = get_guest_cr3_from_dte(gdte);
+    glx      = get_glx_from_dte(gdte);
+    gv       = get_gv_from_dte(gdte);
+
+    unmap_domain_page(gdte);
 
     /* Do not update host dte before gcr3 has been set */
     if ( gcr3_gfn == 0 )
@@ -449,11 +451,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
         return -ENODEV;
     }
 
-    glx = get_glx_from_dte(gdte);
-    gv = get_gv_from_dte(gdte);
-
-    unmap_domain_page(dte_base);
-
     /* Setup host device entry */
     hdom_id = host_domid(d, gdom_id);
     req_id = get_dma_requestor_id(iommu->seg, mbdf);
-- 
1.7.10.4

Re: [PATCH v2 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Jan Beulich]

>>> On 04.12.13 at 17:44, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> @@ -424,12 +424,14 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>                                          sizeof(dev_entry_t), gbdf);
>      ASSERT(mfn_valid(dte_mfn));
>  
> -    dte_base = map_domain_page(dte_mfn);
> -
> -    gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t));
> +    gdte = map_domain_page(dte_mfn) + gbdf % (PAGE_SIZE / sizeof(dev_entry_t));

You would better have avoided the extra cleanup, as the arithmetic
now is different from what it was before (acting on void * now, i.e.
byte granular, instead of in dev_entry_t *).

Jan

[PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Andrew Cooper]

Coverity ID: 1135379

As the code stands, the domain mapping will be leaked on each error path.

The mapping can be for a much shorter period of time, and all the relevent
information can be pulled out at once.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

---

Changes in v3:
 * Dont break the pointer arithmatic on gdte
---
 xen/drivers/passthrough/amd/iommu_guest.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 952600a..c1fa0ff 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -430,6 +430,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
 
     gdom_id  = get_domid_from_dte(gdte);
     gcr3_gfn = get_guest_cr3_from_dte(gdte);
+    glx      = get_glx_from_dte(gdte);
+    gv       = get_gv_from_dte(gdte);
+
+    unmap_domain_page(gdte);
 
     /* Do not update host dte before gcr3 has been set */
     if ( gcr3_gfn == 0 )
@@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
         return -ENODEV;
     }
 
-    glx = get_glx_from_dte(gdte);
-    gv = get_gv_from_dte(gdte);
-
-    unmap_domain_page(dte_base);
-
     /* Setup host device entry */
     hdom_id = host_domid(d, gdom_id);
     req_id = get_dma_requestor_id(iommu->seg, mbdf);
-- 
1.7.10.4

Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Jan Beulich]

>>> On 04.12.13 at 18:59, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Coverity ID: 1135379
> 
> As the code stands, the domain mapping will be leaked on each error path.
> 
> The mapping can be for a much shorter period of time, and all the relevent
> information can be pulled out at once.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> ---
> 
> Changes in v3:
>  * Dont break the pointer arithmatic on gdte
> ---
>  xen/drivers/passthrough/amd/iommu_guest.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c 
> b/xen/drivers/passthrough/amd/iommu_guest.c
> index 952600a..c1fa0ff 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -430,6 +430,10 @@ static int do_invalidate_dte(struct domain *d, 
> cmd_entry_t *cmd)
>  
>      gdom_id  = get_domid_from_dte(gdte);
>      gcr3_gfn = get_guest_cr3_from_dte(gdte);
> +    glx      = get_glx_from_dte(gdte);
> +    gv       = get_gv_from_dte(gdte);
> +
> +    unmap_domain_page(gdte);
>  
>      /* Do not update host dte before gcr3 has been set */
>      if ( gcr3_gfn == 0 )
> @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, 
> cmd_entry_t *cmd)
>          return -ENODEV;
>      }
>  
> -    glx = get_glx_from_dte(gdte);
> -    gv = get_gv_from_dte(gdte);
> -
> -    unmap_domain_page(dte_base);
> -
>      /* Setup host device entry */
>      hdom_id = host_domid(d, gdom_id);
>      req_id = get_dma_requestor_id(iommu->seg, mbdf);
> -- 
> 1.7.10.4

Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Jan Beulich]

>>> On 04.12.13 at 18:59, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Coverity ID: 1135379
> 
> As the code stands, the domain mapping will be leaked on each error path.
> 
> The mapping can be for a much shorter period of time, and all the relevent
> information can be pulled out at once.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Suravee?

> ---
> 
> Changes in v3:
>  * Dont break the pointer arithmatic on gdte
> ---
>  xen/drivers/passthrough/amd/iommu_guest.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c 
> b/xen/drivers/passthrough/amd/iommu_guest.c
> index 952600a..c1fa0ff 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -430,6 +430,10 @@ static int do_invalidate_dte(struct domain *d, 
> cmd_entry_t *cmd)
>  
>      gdom_id  = get_domid_from_dte(gdte);
>      gcr3_gfn = get_guest_cr3_from_dte(gdte);
> +    glx      = get_glx_from_dte(gdte);
> +    gv       = get_gv_from_dte(gdte);
> +
> +    unmap_domain_page(gdte);
>  
>      /* Do not update host dte before gcr3 has been set */
>      if ( gcr3_gfn == 0 )
> @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, 
> cmd_entry_t *cmd)
>          return -ENODEV;
>      }
>  
> -    glx = get_glx_from_dte(gdte);
> -    gv = get_gv_from_dte(gdte);
> -
> -    unmap_domain_page(dte_base);
> -
>      /* Setup host device entry */
>      hdom_id = host_domid(d, gdom_id);
>      req_id = get_dma_requestor_id(iommu->seg, mbdf);
> -- 
> 1.7.10.4

Re: [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page()

[Jan Beulich]

>>> On 04.12.13 at 16:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> This avoids a resource leak and needless playing with the pagetables in the
> case that the page is broken.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>

Keir?

> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> ---
>  xen/common/page_alloc.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 8002bd2..5f484a2 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1652,11 +1652,13 @@ __initcall(pagealloc_keyhandler_init);
>  
>  void scrub_one_page(struct page_info *pg)
>  {
> -    void *p = __map_domain_page(pg);
> +    void *p;
>  
>      if ( unlikely(pg->count_info & PGC_broken) )
>          return;
>  
> +    p = __map_domain_page(pg);
> +
>  #ifndef NDEBUG
>      /* Avoid callers relying on allocations returning zeroed pages. */
>      memset(p, 0xc2, PAGE_SIZE);
> -- 
> 1.7.10.4

Re: [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page()

[Keir Fraser]

On 09/12/2013 10:09, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 04.12.13 at 16:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> This avoids a resource leak and needless playing with the pagetables in the
>> case that the page is broken.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
> 
> Keir?

Reviewed-by: Keir Fraser <keir@xen.org>

>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>> ---
>>  xen/common/page_alloc.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 8002bd2..5f484a2 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1652,11 +1652,13 @@ __initcall(pagealloc_keyhandler_init);
>>  
>>  void scrub_one_page(struct page_info *pg)
>>  {
>> -    void *p = __map_domain_page(pg);
>> +    void *p;
>>  
>>      if ( unlikely(pg->count_info & PGC_broken) )
>>          return;
>>  
>> +    p = __map_domain_page(pg);
>> +
>>  #ifndef NDEBUG
>>      /* Avoid callers relying on allocations returning zeroed pages. */
>>      memset(p, 0xc2, PAGE_SIZE);
>> -- 
>> 1.7.10.4
> 
> 
> 

Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Suravee Suthikulanit]

On 12/4/2013 11:59 AM, Andrew Cooper wrote:
> Coverity ID: 1135379
>
> As the code stands, the domain mapping will be leaked on each error path.
>
> The mapping can be for a much shorter period of time, and all the relevent
> information can be pulled out at once.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> ---
>
> Changes in v3:
>   * Dont break the pointer arithmatic on gdte
> ---
>   xen/drivers/passthrough/amd/iommu_guest.c |    9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
> index 952600a..c1fa0ff 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -430,6 +430,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>   
>       gdom_id  = get_domid_from_dte(gdte);
>       gcr3_gfn = get_guest_cr3_from_dte(gdte);
> +    glx      = get_glx_from_dte(gdte);
> +    gv       = get_gv_from_dte(gdte);
> +
> +    unmap_domain_page(gdte);
Shouldn't this be "unmap_domain_page (dte_base)" instead?

>   
>       /* Do not update host dte before gcr3 has been set */
>       if ( gcr3_gfn == 0 )
> @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>           return -ENODEV;
>       }
>   
> -    glx = get_glx_from_dte(gdte);
> -    gv = get_gv_from_dte(gdte);
> -
> -    unmap_domain_page(dte_base);
> -
>       /* Setup host device entry */
>       hdom_id = host_domid(d, gdom_id);
>       req_id = get_dma_requestor_id(iommu->seg, mbdf);
Also, the comment saying "/* Read guest dte information */ " should 
probably be moved as well.

Suravee

Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Andrew Cooper]

On 09/12/13 18:33, Suravee Suthikulanit wrote:
> On 12/4/2013 11:59 AM, Andrew Cooper wrote:
>> Coverity ID: 1135379
>>
>> As the code stands, the domain mapping will be leaked on each error
>> path.
>>
>> The mapping can be for a much shorter period of time, and all the
>> relevent
>> information can be pulled out at once.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> ---
>>
>> Changes in v3:
>>   * Dont break the pointer arithmatic on gdte
>> ---
>>   xen/drivers/passthrough/amd/iommu_guest.c |    9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c
>> b/xen/drivers/passthrough/amd/iommu_guest.c
>> index 952600a..c1fa0ff 100644
>> --- a/xen/drivers/passthrough/amd/iommu_guest.c
>> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
>> @@ -430,6 +430,10 @@ static int do_invalidate_dte(struct domain *d,
>> cmd_entry_t *cmd)
>>         gdom_id  = get_domid_from_dte(gdte);
>>       gcr3_gfn = get_guest_cr3_from_dte(gdte);
>> +    glx      = get_glx_from_dte(gdte);
>> +    gv       = get_gv_from_dte(gdte);
>> +
>> +    unmap_domain_page(gdte);
> Shouldn't this be "unmap_domain_page (dte_base)" instead?

Probably should be.

>
>>         /* Do not update host dte before gcr3 has been set */
>>       if ( gcr3_gfn == 0 )
>> @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d,
>> cmd_entry_t *cmd)
>>           return -ENODEV;
>>       }
>>   -    glx = get_glx_from_dte(gdte);
>> -    gv = get_gv_from_dte(gdte);
>> -
>> -    unmap_domain_page(dte_base);
>> -
>>       /* Setup host device entry */
>>       hdom_id = host_domid(d, gdom_id);
>>       req_id = get_dma_requestor_id(iommu->seg, mbdf);
> Also, the comment saying "/* Read guest dte information */ " should
> probably be moved as well.
>
> Suravee
>

Sure - v4 on its way.

~Andrew

[PATCH v4 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Andrew Cooper]

Coverity ID: 1135379

As the code stands, the domain mapping will be leaked on each error path.

The mapping can be for a much shorter period of time, and all the relevent
information can be pulled out at once.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
Reviewed-by: Jan Beulich <JBeulich@suse.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

---

Changes in v4:
 * Move comment, and unmap the base pointer.
---
 xen/drivers/passthrough/amd/iommu_guest.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 952600a..477de20 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -424,12 +424,17 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
                                         sizeof(dev_entry_t), gbdf);
     ASSERT(mfn_valid(dte_mfn));
 
+    /* Read guest dte information */
     dte_base = map_domain_page(dte_mfn);
 
     gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t));
 
     gdom_id  = get_domid_from_dte(gdte);
     gcr3_gfn = get_guest_cr3_from_dte(gdte);
+    glx      = get_glx_from_dte(gdte);
+    gv       = get_gv_from_dte(gdte);
+
+    unmap_domain_page(dte_base);
 
     /* Do not update host dte before gcr3 has been set */
     if ( gcr3_gfn == 0 )
@@ -440,7 +445,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
 
     ASSERT(mfn_valid(gcr3_mfn));
 
-    /* Read guest dte information */
     iommu = find_iommu_for_device(0, mbdf);
     if ( !iommu )
     {
@@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
         return -ENODEV;
     }
 
-    glx = get_glx_from_dte(gdte);
-    gv = get_gv_from_dte(gdte);
-
-    unmap_domain_page(dte_base);
-
     /* Setup host device entry */
     hdom_id = host_domid(d, gdom_id);
     req_id = get_dma_requestor_id(iommu->seg, mbdf);
-- 
1.7.10.4

Re: [PATCH v4 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()

[Suravee Suthikulpanit]

[-- Attachment #1.1: Type: text/plain, Size: 2319 bytes --]

On 12/9/2013 12:41 PM, Andrew Cooper wrote:
> Coverity ID: 1135379
>
> As the code stands, the domain mapping will be leaked on each error path.
>
> The mapping can be for a much shorter period of time, and all the relevent
> information can be pulled out at once.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> Reviewed-by: Jan Beulich <JBeulich@suse.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> ---
>
> Changes in v4:
>   * Move comment, and unmap the base pointer.
> ---
>   xen/drivers/passthrough/amd/iommu_guest.c |   11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
> index 952600a..477de20 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -424,12 +424,17 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>                                           sizeof(dev_entry_t), gbdf);
>       ASSERT(mfn_valid(dte_mfn));
>   
> +    /* Read guest dte information */
>       dte_base = map_domain_page(dte_mfn);
>   
>       gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t));
>   
>       gdom_id  = get_domid_from_dte(gdte);
>       gcr3_gfn = get_guest_cr3_from_dte(gdte);
> +    glx      = get_glx_from_dte(gdte);
> +    gv       = get_gv_from_dte(gdte);
> +
> +    unmap_domain_page(dte_base);
>   
>       /* Do not update host dte before gcr3 has been set */
>       if ( gcr3_gfn == 0 )
> @@ -440,7 +445,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>   
>       ASSERT(mfn_valid(gcr3_mfn));
>   
> -    /* Read guest dte information */
>       iommu = find_iommu_for_device(0, mbdf);
>       if ( !iommu )
>       {
> @@ -449,11 +453,6 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>           return -ENODEV;
>       }
>   
> -    glx = get_glx_from_dte(gdte);
> -    gv = get_gv_from_dte(gdte);
> -
> -    unmap_domain_page(dte_base);
> -
>       /* Setup host device entry */
>       hdom_id = host_domid(d, gdom_id);
>       req_id = get_dma_requestor_id(iommu->seg, mbdf);
Reviewed and Tested.

Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Thanks,

Suravee

[-- Attachment #1.2: Type: text/html, Size: 3085 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel