Xen devel - Dec 2013 - [Patch] x86/mm: Prevent leaking domain mappings in paging_log_dirty_op()

Coverity ID: 1135374 1135375 1135376 1135377

If {copy_to,clear}_guest_offset() fails, we would leak the domain mappings for
l4 thru l1.

Fixing this requires having conditional unmaps on the faulting path, which in
turn requires explicitly initialising the pointers to NULL because of the
early ENOMEM exit.

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/arch/x86/mm/paging.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 4ba7669..3530766 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -330,8 +330,8 @@ int paging_log_dirty_op(struct domain *d, struct
xen_domctl_shadow_op *sc)
 {
     int rv = 0, clean = 0, peek = 1;
     unsigned long pages = 0;
-    mfn_t *l4, *l3, *l2;
-    unsigned long *l1;
+    mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
+    unsigned long *l1 = NULL;
     int i4, i3, i2;
 
     domain_pause(d);
@@ -432,6 +432,15 @@ int paging_log_dirty_op(struct domain *d, struct
xen_domctl_shadow_op *sc)
     return rv;
 
  out:
+    if ( l1 )
+        unmap_domain_page(l1);
+    if ( l2 )
+        unmap_domain_page(l2);
+    if ( l3 )
+        unmap_domain_page(l3);
+    if ( l4 )
+        unmap_domain_page(l4);
+
     paging_unlock(d);
     domain_unpause(d);
     return rv;
-- 
1.7.10.4

>>> On 10.12.13 at 14:53, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> Coverity ID: 1135374 1135375 1135376 1135377
> 
> If {copy_to,clear}_guest_offset() fails, we would leak the domain mappings 
> for
> l4 thru l1.
> 
> Fixing this requires having conditional unmaps on the faulting path, which 
> in
> turn requires explicitly initialising the pointers to NULL because of the
> early ENOMEM exit.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

with a minor comment:
> @@ -432,6 +432,15 @@ int paging_log_dirty_op(struct domain *d, struct
xen_domctl_shadow_op *sc)
>      return rv;
>  
>   out:
> +    if ( l1 )
> +        unmap_domain_page(l1);
> +    if ( l2 )
> +        unmap_domain_page(l2);
> +    if ( l3 )
> +        unmap_domain_page(l3);
> +    if ( l4 )
> +        unmap_domain_page(l4);
> +
>      paging_unlock(d);
>      domain_unpause(d);
>      return rv;
While on an error path, it would nevertheless seem better to do the
unmaps after the unlock/unpause.

Jan

Coverity ID: 1135374 1135375 1135376 1135377

If {copy_to,clear}_guest_offset() fails, we would leak the domain mappings for
l4 thru l1.

Fixing this requires having conditional unmaps on the faulting path, which in
turn requires explicitly initialising the pointers to NULL because of the
early ENOMEM exit.

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>

---

Changes in v2:
 * Reorder unmaps until after the unlock/unpause (Suggested by Jan)
---
 xen/arch/x86/mm/paging.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 4ba7669..21344e5 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -330,8 +330,8 @@ int paging_log_dirty_op(struct domain *d, struct
xen_domctl_shadow_op *sc)
 {
     int rv = 0, clean = 0, peek = 1;
     unsigned long pages = 0;
-    mfn_t *l4, *l3, *l2;
-    unsigned long *l1;
+    mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
+    unsigned long *l1 = NULL;
     int i4, i3, i2;
 
     domain_pause(d);
@@ -434,6 +434,16 @@ int paging_log_dirty_op(struct domain *d, struct
xen_domctl_shadow_op *sc)
  out:
     paging_unlock(d);
     domain_unpause(d);
+
+    if ( l1 )
+        unmap_domain_page(l1);
+    if ( l2 )
+        unmap_domain_page(l2);
+    if ( l3 )
+        unmap_domain_page(l3);
+    if ( l4 )
+        unmap_domain_page(l4);
+
     return rv;
 }
 
-- 
1.7.10.4