Ian Campbell
2013-Jan-04 16:52 UTC
[PATCH] libxc: x86: ensure that the initial mapping fits into the guest''s memory
On Fri, 2013-01-04 at 16:01 +0000, Xen.org security team wrote:> Xen Security Advisory CVE-2013-0154 / XSA-37 > > Hypervisor crash due to incorrect ASSERT (debug build only)This issue was exposed/triggered by a libxc bug, the fix for which is below. 8<--------------------------------- # HG changeset patch # User Ian Campbell <ijc@hellion.org.uk> # Date 1357318325 0 # Node ID 84dea8458c36e6ee7d9347d32455cbeb5595825b # Parent 7a61269a0c1ab33cc93b47f88d760f0d1f88eaab libxc: x86: ensure that the initial mapping fits into the guest''s memory In particular we need to check that adding 512KB of slack and rounding up to a 4MB boundary do not overflow the guest''s memory allocation. Otherwise we run off the end of the p2m when building the guest''s initial page tables and populate them with garbage. Wei noticed this when build tiny (2MB) mini-os domains. Reported-by: Wei Liu <Wei.Liu2@citrix.com> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com> diff -r 7a61269a0c1a -r 84dea8458c36 tools/libxc/xc_dom_core.c --- a/tools/libxc/xc_dom_core.c Thu Jan 03 16:31:44 2013 +0000 +++ b/tools/libxc/xc_dom_core.c Fri Jan 04 16:52:05 2013 +0000 @@ -873,7 +873,8 @@ int xc_dom_build_image(struct xc_dom_ima goto err; if ( dom->arch_hooks->count_pgtables ) { - dom->arch_hooks->count_pgtables(dom); + if ( dom->arch_hooks->count_pgtables(dom) != 0 ) + goto err; if ( (dom->pgtables > 0) && (xc_dom_alloc_segment(dom, &dom->pgtables_seg, "page tables", 0, dom->pgtables * page_size) != 0) ) diff -r 7a61269a0c1a -r 84dea8458c36 tools/libxc/xc_dom_x86.c --- a/tools/libxc/xc_dom_x86.c Thu Jan 03 16:31:44 2013 +0000 +++ b/tools/libxc/xc_dom_x86.c Fri Jan 04 16:52:05 2013 +0000 @@ -91,6 +91,15 @@ static int count_pgtables(struct xc_dom_ { try_virt_end = round_up(dom->virt_alloc_end + pages * PAGE_SIZE_X86, bits_to_mask(22)); /* 4MB alignment */ + + if ( (try_virt_end >> PAGE_SHIFT_X86) > dom->total_pages ) + { + xc_dom_panic(dom->xch, XC_OUT_OF_MEMORY, + "%s: not enough memory for initial mapping", + __FUNCTION__); + return -ENOMEM; + } + dom->pg_l4 nr_page_tables(dom, dom->parms.virt_base, try_virt_end, l4_bits); dom->pg_l3 =
Jan Beulich
2013-Jan-07 07:00 UTC
Re: [PATCH] libxc: x86: ensure that the initial mapping fits into the guest''s memory
>>> Ian Campbell <Ian.Campbell@citrix.com> 01/04/13 5:53 PM >>> >libxc: x86: ensure that the initial mapping fits into the guest''s memory > >In particular we need to check that adding 512KB of slack and >rounding up to a 4MB boundary do not overflow the guest''s memory >allocation. Otherwise we run off the end of the p2m when building the >guest''s initial page tables and populate them with garbage.Sadly our testing found this to cause SLE11 SP2 PV guests to not start anymore (in its 4.1.x backported incarnation). I didn''t get around yet to check whether in the (apparently trivial) backport I overlooked something; will do as soon as I get to the office. Jan
Jan Beulich
2013-Jan-07 10:35 UTC
Re: [PATCH] libxc: x86: ensure that the initial mapping fits into the guest''s memory
>>> On 07.01.13 at 08:00, Jan Beulich wrote: >>>> Ian Campbell <Ian.Campbell@citrix.com> 01/04/13 5:53 PM >>> >>libxc: x86: ensure that the initial mapping fits into the guest''s memory >> >>In particular we need to check that adding 512KB of slack and >>rounding up to a 4MB boundary do not overflow the guest''s memory >>allocation. Otherwise we run off the end of the p2m when building the >>guest''s initial page tables and populate them with garbage. > > Sadly our testing found this to cause SLE11 SP2 PV guests to not start > anymore (in its 4.1.x backported incarnation). I didn''t get around yet to > check whether in the (apparently trivial) backport I overlooked something; > will do as soon as I get to the office.Switching the added panic invocation to xc_dom_panic(dom->xch, XC_OUT_OF_MEMORY, "%s: not enough memory for initial mapping (%#"PRIx64" > %#"PRIpfn")", __FUNCTION__, try_virt_end >> PAGE_SHIFT_X86, dom->total_pages); I see (with xend on 4.1.3) xc: error: panic: xc_dom_x86.c:100: count_pgtables: not enough memory for initial mapping (0xffffffff81bff > 0x80000): Out of memory Did this really work for you? The 4.1.3 xl doesn''t really want to work for me, so I can''t directly cross check whether there''s a behavioral difference between the two, but looking at an older log the virtual addresses reported for virt_alloc_end look similar. Afaict you need to subtract dom->parms.virt_base from try_virt_end. Jan
Ian Campbell
2013-Jan-07 10:37 UTC
Re: [PATCH] libxc: x86: ensure that the initial mapping fits into the guest''s memory
On Mon, 2013-01-07 at 10:35 +0000, Jan Beulich wrote:> >>> On 07.01.13 at 08:00, Jan Beulich wrote: > >>>> Ian Campbell <Ian.Campbell@citrix.com> 01/04/13 5:53 PM >>> > >>libxc: x86: ensure that the initial mapping fits into the guest''s memory > >> > >>In particular we need to check that adding 512KB of slack and > >>rounding up to a 4MB boundary do not overflow the guest''s memory > >>allocation. Otherwise we run off the end of the p2m when building the > >>guest''s initial page tables and populate them with garbage. > > > > Sadly our testing found this to cause SLE11 SP2 PV guests to not start > > anymore (in its 4.1.x backported incarnation). I didn''t get around yet to > > check whether in the (apparently trivial) backport I overlooked something; > > will do as soon as I get to the office. > > Switching the added panic invocation to > > xc_dom_panic(dom->xch, XC_OUT_OF_MEMORY, > "%s: not enough memory for initial mapping (%#"PRIx64" > %#"PRIpfn")", > __FUNCTION__, try_virt_end >> PAGE_SHIFT_X86, > dom->total_pages); > > I see (with xend on 4.1.3) > > xc: error: panic: xc_dom_x86.c:100: count_pgtables: not enough memory for initial mapping (0xffffffff81bff > 0x80000): Out of memory > > Did this really work for you?It did but I must confess I only tested with the mini-os test domain, since that was what the initial bug was reported about and I stupidly didn''t think to test with a "real" kernel.> The 4.1.3 xl doesn''t really want to work > for me, so I can''t directly cross check whether there''s a behavioral > difference between the two, but looking at an older log the virtual > addresses reported for virt_alloc_end look similar. Afaict you need > to subtract dom->parms.virt_base from try_virt_end.I bet virt_base == 0 for the mini-os kernel I tried. I''ll respin and retest. Thanks and sorry for the cockup, Ian.
Jan Beulich
2013-Jan-07 11:05 UTC
Re: [PATCH] libxc: x86: ensure that the initial mapping fits into the guest''s memory
>>> On 07.01.13 at 11:37, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Mon, 2013-01-07 at 10:35 +0000, Jan Beulich wrote: >> >>> On 07.01.13 at 08:00, Jan Beulich wrote: >> >>>> Ian Campbell <Ian.Campbell@citrix.com> 01/04/13 5:53 PM >>> >> >>libxc: x86: ensure that the initial mapping fits into the guest''s memory >> >> >> >>In particular we need to check that adding 512KB of slack and >> >>rounding up to a 4MB boundary do not overflow the guest''s memory >> >>allocation. Otherwise we run off the end of the p2m when building the >> >>guest''s initial page tables and populate them with garbage. >> > >> > Sadly our testing found this to cause SLE11 SP2 PV guests to not start >> > anymore (in its 4.1.x backported incarnation). I didn''t get around yet to >> > check whether in the (apparently trivial) backport I overlooked something; >> > will do as soon as I get to the office. >> >> Switching the added panic invocation to >> >> xc_dom_panic(dom->xch, XC_OUT_OF_MEMORY, >> "%s: not enough memory for initial mapping > (%#"PRIx64" > %#"PRIpfn")", >> __FUNCTION__, try_virt_end >> PAGE_SHIFT_X86, >> dom->total_pages); >> >> I see (with xend on 4.1.3) >> >> xc: error: panic: xc_dom_x86.c:100: count_pgtables: not enough memory for > initial mapping (0xffffffff81bff > 0x80000): Out of memory >> >> Did this really work for you? > > It did but I must confess I only tested with the mini-os test domain, > since that was what the initial bug was reported about and I stupidly > didn''t think to test with a "real" kernel. > >> The 4.1.3 xl doesn''t really want to work >> for me, so I can''t directly cross check whether there''s a behavioral >> difference between the two, but looking at an older log the virtual >> addresses reported for virt_alloc_end look similar. Afaict you need >> to subtract dom->parms.virt_base from try_virt_end. > > I bet virt_base == 0 for the mini-os kernel I tried. I''ll respin and > retest.This is what works for me (also added printing of the relevant value, and dropping the unchanged parts of the patch): --- a/tools/libxc/xc_dom_x86.c +++ b/tools/libxc/xc_dom_x86.c @@ -82,6 +82,7 @@ static int count_pgtables(struct xc_dom_ { int pages, extra_pages; xen_vaddr_t try_virt_end; + xen_pfn_t try_pfn_end; extra_pages = dom->alloc_bootstack ? 1 : 0; extra_pages += dom->extra_pages; @@ -91,6 +92,16 @@ static int count_pgtables(struct xc_dom_ { try_virt_end = round_up(dom->virt_alloc_end + pages * PAGE_SIZE_X86, bits_to_mask(22)); /* 4MB alignment */ + try_pfn_end = (try_virt_end - dom->parms.virt_base) >> PAGE_SHIFT_X86; + + if ( try_pfn_end > dom->total_pages ) + { + xc_dom_panic(dom->xch, XC_OUT_OF_MEMORY, + "%s: not enough memory for initial mapping (%#"PRIpfn" > %#"PRIpfn")", + __FUNCTION__, try_pfn_end, dom->total_pages); + return -ENOMEM; + } + dom->pg_l4 nr_page_tables(dom, dom->parms.virt_base, try_virt_end, l4_bits); dom->pg_l3 Jan
Ian Campbell
2013-Jan-07 11:39 UTC
Re: [PATCH] libxc: x86: ensure that the initial mapping fits into the guest''s memory
On Mon, 2013-01-07 at 11:05 +0000, Jan Beulich wrote:> >> The 4.1.3 xl doesn''t really want to work > >> for me, so I can''t directly cross check whether there''s a behavioral > >> difference between the two, but looking at an older log the virtual > >> addresses reported for virt_alloc_end look similar. Afaict you need > >> to subtract dom->parms.virt_base from try_virt_end. > > > > I bet virt_base == 0 for the mini-os kernel I tried. I''ll respin and > > retest. > > This is what works for me (also added printing of the relevant value, > and dropping the unchanged parts of the patch):This looks sensible to me, thanks. I shall respin the complete patch shortly. Ian.> > --- a/tools/libxc/xc_dom_x86.c > +++ b/tools/libxc/xc_dom_x86.c > @@ -82,6 +82,7 @@ static int count_pgtables(struct xc_dom_ > { > int pages, extra_pages; > xen_vaddr_t try_virt_end; > + xen_pfn_t try_pfn_end; > > extra_pages = dom->alloc_bootstack ? 1 : 0; > extra_pages += dom->extra_pages; > @@ -91,6 +92,16 @@ static int count_pgtables(struct xc_dom_ > { > try_virt_end = round_up(dom->virt_alloc_end + pages * PAGE_SIZE_X86, > bits_to_mask(22)); /* 4MB alignment */ > + try_pfn_end = (try_virt_end - dom->parms.virt_base) >> PAGE_SHIFT_X86; > + > + if ( try_pfn_end > dom->total_pages ) > + { > + xc_dom_panic(dom->xch, XC_OUT_OF_MEMORY, > + "%s: not enough memory for initial mapping (%#"PRIpfn" > %#"PRIpfn")", > + __FUNCTION__, try_pfn_end, dom->total_pages); > + return -ENOMEM; > + } > + > dom->pg_l4 > nr_page_tables(dom, dom->parms.virt_base, try_virt_end, l4_bits); > dom->pg_l3 > > Jan >
Ian Campbell
2013-Jan-07 13:37 UTC
[PATCH V2] libxc: x86: ensure that the initial mapping fits into the guest''s memory
On Mon, 2013-01-07 at 11:39 +0000, Ian Campbell wrote:> I shall respin the complete patch shortly.8<---------------------------------- # HG changeset patch # User Ian Campbell <ijc@hellion.org.uk> # Date 1357565755 0 # Node ID c95c8ea2424b928be44550f29119f5e58f628f8b # Parent 7a61269a0c1ab33cc93b47f88d760f0d1f88eaab libxc: x86: ensure that the initial mapping fits into the guest''s memory In particular we need to check that adding 512KB of slack and rounding up to a 4MB boundary do not overflow the guest''s memory allocation. Otherwise we run off the end of the p2m when building the guest''s initial page tables and populate them with garbage. Wei noticed this when build tiny (2MB) mini-os domains. Reported-by: Wei Liu <Wei.Liu2@citrix.com> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com> --- v2: Correctly account for virt_base, thanks Jan. Now tested with 32 and 64 bit pvops kernels. diff -r 7a61269a0c1a -r c95c8ea2424b tools/libxc/xc_dom_core.c --- a/tools/libxc/xc_dom_core.c Thu Jan 03 16:31:44 2013 +0000 +++ b/tools/libxc/xc_dom_core.c Mon Jan 07 13:35:55 2013 +0000 @@ -873,7 +873,8 @@ int xc_dom_build_image(struct xc_dom_ima goto err; if ( dom->arch_hooks->count_pgtables ) { - dom->arch_hooks->count_pgtables(dom); + if ( dom->arch_hooks->count_pgtables(dom) != 0 ) + goto err; if ( (dom->pgtables > 0) && (xc_dom_alloc_segment(dom, &dom->pgtables_seg, "page tables", 0, dom->pgtables * page_size) != 0) ) diff -r 7a61269a0c1a -r c95c8ea2424b tools/libxc/xc_dom_x86.c --- a/tools/libxc/xc_dom_x86.c Thu Jan 03 16:31:44 2013 +0000 +++ b/tools/libxc/xc_dom_x86.c Mon Jan 07 13:35:55 2013 +0000 @@ -82,6 +82,7 @@ static int count_pgtables(struct xc_dom_ { int pages, extra_pages; xen_vaddr_t try_virt_end; + xen_pfn_t try_pfn_end; extra_pages = dom->alloc_bootstack ? 1 : 0; extra_pages += dom->extra_pages; @@ -91,6 +92,17 @@ static int count_pgtables(struct xc_dom_ { try_virt_end = round_up(dom->virt_alloc_end + pages * PAGE_SIZE_X86, bits_to_mask(22)); /* 4MB alignment */ + + try_pfn_end = (try_virt_end - dom->parms.virt_base) >> PAGE_SHIFT_X86; + + if ( try_pfn_end > dom->total_pages ) + { + xc_dom_panic(dom->xch, XC_OUT_OF_MEMORY, + "%s: not enough memory for initial mapping (%#"PRIpfn" > %#"PRIpfn")", + __FUNCTION__, try_pfn_end, dom->total_pages); + return -ENOMEM; + } + dom->pg_l4 nr_page_tables(dom, dom->parms.virt_base, try_virt_end, l4_bits); dom->pg_l3 =