* [Patch] x86/mm: Prevent leaking domain mappings in paging_log_dirty_op()
@ 2013-12-10 13:53 Andrew Cooper
2013-12-10 14:45 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2013-12-10 13:53 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich, Tim Deegan
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch] x86/mm: Prevent leaking domain mappings in paging_log_dirty_op()
2013-12-10 13:53 [Patch] x86/mm: Prevent leaking domain mappings in paging_log_dirty_op() Andrew Cooper
@ 2013-12-10 14:45 ` Jan Beulich
2013-12-10 14:50 ` [Patch v2] " Andrew Cooper
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2013-12-10 14:45 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Tim Deegan
>>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Patch v2] x86/mm: Prevent leaking domain mappings in paging_log_dirty_op()
2013-12-10 14:45 ` Jan Beulich
@ 2013-12-10 14:50 ` Andrew Cooper
2013-12-17 9:14 ` Ping: " Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2013-12-10 14:50 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Tim Deegan
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Ping: [Patch v2] x86/mm: Prevent leaking domain mappings in paging_log_dirty_op()
2013-12-10 14:50 ` [Patch v2] " Andrew Cooper
@ 2013-12-17 9:14 ` Jan Beulich
2013-12-17 10:30 ` Tim Deegan
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2013-12-17 9:14 UTC (permalink / raw)
To: Tim Deegan; +Cc: Andrew Cooper, Keir Fraser, Xen-devel
>>> On 10.12.13 at 15:50, 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>
> CC: Keir Fraser <keir@xen.org>
> Reviewed-by: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
Tim?
> ---
>
> 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
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Ping: [Patch v2] x86/mm: Prevent leaking domain mappings in paging_log_dirty_op()
2013-12-17 9:14 ` Ping: " Jan Beulich
@ 2013-12-17 10:30 ` Tim Deegan
0 siblings, 0 replies; 5+ messages in thread
From: Tim Deegan @ 2013-12-17 10:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Keir Fraser, Xen-devel
At 09:14 +0000 on 17 Dec (1387268042), Jan Beulich wrote:
> >>> On 10.12.13 at 15:50, 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>
> > CC: Keir Fraser <keir@xen.org>
> > Reviewed-by: Jan Beulich <JBeulich@suse.com>
> > CC: Tim Deegan <tim@xen.org>
>
> Tim?
Sorry, I hadn't seen v2. I think I'd prefer the unmap to be done at
the one 'goto out', where we know that all four are mapped (so no
initializers needed either; doing it this way may lead to double-frees
if any more 'goto out's get added. But taking into account Jan's
comment about dropping the locks first, I guess that's OK.
Acked-by: 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
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-12-17 10:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-10 13:53 [Patch] x86/mm: Prevent leaking domain mappings in paging_log_dirty_op() Andrew Cooper
2013-12-10 14:45 ` Jan Beulich
2013-12-10 14:50 ` [Patch v2] " Andrew Cooper
2013-12-17 9:14 ` Ping: " Jan Beulich
2013-12-17 10:30 ` Tim Deegan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).