Andrew Cooper
2013-Dec-04 15:09 UTC
[PATCH 0/2] Coverity fixes for map_domain_page() mismatches
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>
Andrew Cooper
2013-Dec-04 15:09 UTC
[PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
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
Andrew Cooper
2013-Dec-04 15:09 UTC
[PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page()
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
Jan Beulich
2013-Dec-04 15:22 UTC
Re: [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
>>> 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
Jan Beulich
2013-Dec-04 15:23 UTC
Re: [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page()
>>> 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
Andrew Cooper
2013-Dec-04 15:30 UTC
Re: [PATCH 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
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
Andrew Cooper
2013-Dec-04 16:44 UTC
[PATCH v2 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
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
Jan Beulich
2013-Dec-04 16:49 UTC
Re: [PATCH v2 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
>>> 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
Andrew Cooper
2013-Dec-04 17:59 UTC
[PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
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
Jan Beulich
2013-Dec-06 08:37 UTC
Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
>>> 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
Jan Beulich
2013-Dec-09 10:08 UTC
Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
>>> 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
Jan Beulich
2013-Dec-09 10:09 UTC
Re: [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page()
>>> 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
Keir Fraser
2013-Dec-09 11:02 UTC
Re: [PATCH 2/2] xen/page_alloc: Defer the domain mapping in scrub_one_page()
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 > > >
Suravee Suthikulanit
2013-Dec-09 18:33 UTC
Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
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
Andrew Cooper
2013-Dec-09 18:34 UTC
Re: [PATCH v3 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
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
Andrew Cooper
2013-Dec-09 18:41 UTC
[PATCH v4 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
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
Suravee Suthikulpanit
2013-Dec-09 20:02 UTC
Re: [PATCH v4 1/2] amd/passthrough: Do not leak domain mappings from do_invalidate_dte()
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel