Message ID | 1372438702-20491-3-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
On 2013-06-28 18:58, Paolo Bonzini wrote: > phys_sections_clear is invoked after the dispatch tree has been > destroyed. You mean destroy_all_mappings called by mem_begin vs. phys_sections_clear called by core_begin, right? > This leaves a window where phys_sections_nb > 0 but the > subpages are not valid anymore, which is a recipe for use-after-free > bugs. > > Move the destruction of subpages in phys_sections_clear. What ensures that we still destroy the subpages when an address space is cleaned up (address_space_destroy_dispatch)? The fact that an address space has to be / is de facto empty when destroying it? That question also applies on the next patch. Jan > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > exec.c | 27 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 14 deletions(-) > > diff --git a/exec.c b/exec.c > index 1118587..e7eadf5 100644 > --- a/exec.c > +++ b/exec.c > @@ -762,17 +762,6 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env, > static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end, > uint16_t section); > static subpage_t *subpage_init(AddressSpace *as, hwaddr base); > -static void destroy_page_desc(uint16_t section_index) > -{ > - MemoryRegionSection *section = &phys_sections[section_index]; > - MemoryRegion *mr = section->mr; > - > - if (mr->subpage) { > - subpage_t *subpage = container_of(mr, subpage_t, iomem); > - memory_region_destroy(&subpage->iomem); > - g_free(subpage); > - } > -} > > static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level) > { > @@ -787,8 +776,6 @@ static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level) > for (i = 0; i < L2_SIZE; ++i) { > if (!p[i].is_leaf) { > destroy_l2_mapping(&p[i], level - 1); > - } else { > - destroy_page_desc(p[i].ptr); > } > } > lp->is_leaf = 0; > @@ -818,9 +805,21 @@ static uint16_t phys_section_add(MemoryRegionSection *section) > return phys_sections_nb++; > } > > +static void phys_section_destroy(MemoryRegion *mr) > +{ > + if (mr->subpage) { > + subpage_t *subpage = container_of(mr, subpage_t, iomem); > + memory_region_destroy(&subpage->iomem); > + g_free(subpage); > + } > +} > + > static void phys_sections_clear(void) > { > - phys_sections_nb = 0; > + while (phys_sections_nb > 0) { > + MemoryRegionSection *section = &phys_sections[--phys_sections_nb]; > + phys_section_destroy(section->mr); > + } > } > > static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section) >
Il 01/07/2013 16:14, Jan Kiszka ha scritto: > On 2013-06-28 18:58, Paolo Bonzini wrote: >> phys_sections_clear is invoked after the dispatch tree has been >> destroyed. > > You mean destroy_all_mappings called by mem_begin vs. > phys_sections_clear called by core_begin, right? > >> This leaves a window where phys_sections_nb > 0 but the >> subpages are not valid anymore, which is a recipe for use-after-free >> bugs. >> >> Move the destruction of subpages in phys_sections_clear. > > What ensures that we still destroy the subpages when an address space is > cleaned up (address_space_destroy_dispatch)? The fact that an address > space has to be / is de facto empty when destroying it? Yes, the address space is emptied at destruction time. See memory_region_transaction_begin(); as->root = NULL; memory_region_transaction_commit(); in address_space_destroy. Paolo > Jan > >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> exec.c | 27 +++++++++++++-------------- >> 1 file changed, 13 insertions(+), 14 deletions(-) >> >> diff --git a/exec.c b/exec.c >> index 1118587..e7eadf5 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -762,17 +762,6 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env, >> static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end, >> uint16_t section); >> static subpage_t *subpage_init(AddressSpace *as, hwaddr base); >> -static void destroy_page_desc(uint16_t section_index) >> -{ >> - MemoryRegionSection *section = &phys_sections[section_index]; >> - MemoryRegion *mr = section->mr; >> - >> - if (mr->subpage) { >> - subpage_t *subpage = container_of(mr, subpage_t, iomem); >> - memory_region_destroy(&subpage->iomem); >> - g_free(subpage); >> - } >> -} >> >> static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level) >> { >> @@ -787,8 +776,6 @@ static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level) >> for (i = 0; i < L2_SIZE; ++i) { >> if (!p[i].is_leaf) { >> destroy_l2_mapping(&p[i], level - 1); >> - } else { >> - destroy_page_desc(p[i].ptr); >> } >> } >> lp->is_leaf = 0; >> @@ -818,9 +805,21 @@ static uint16_t phys_section_add(MemoryRegionSection *section) >> return phys_sections_nb++; >> } >> >> +static void phys_section_destroy(MemoryRegion *mr) >> +{ >> + if (mr->subpage) { >> + subpage_t *subpage = container_of(mr, subpage_t, iomem); >> + memory_region_destroy(&subpage->iomem); >> + g_free(subpage); >> + } >> +} >> + >> static void phys_sections_clear(void) >> { >> - phys_sections_nb = 0; >> + while (phys_sections_nb > 0) { >> + MemoryRegionSection *section = &phys_sections[--phys_sections_nb]; >> + phys_section_destroy(section->mr); >> + } >> } >> >> static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section) >> >
diff --git a/exec.c b/exec.c index 1118587..e7eadf5 100644 --- a/exec.c +++ b/exec.c @@ -762,17 +762,6 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env, static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end, uint16_t section); static subpage_t *subpage_init(AddressSpace *as, hwaddr base); -static void destroy_page_desc(uint16_t section_index) -{ - MemoryRegionSection *section = &phys_sections[section_index]; - MemoryRegion *mr = section->mr; - - if (mr->subpage) { - subpage_t *subpage = container_of(mr, subpage_t, iomem); - memory_region_destroy(&subpage->iomem); - g_free(subpage); - } -} static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level) { @@ -787,8 +776,6 @@ static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level) for (i = 0; i < L2_SIZE; ++i) { if (!p[i].is_leaf) { destroy_l2_mapping(&p[i], level - 1); - } else { - destroy_page_desc(p[i].ptr); } } lp->is_leaf = 0; @@ -818,9 +805,21 @@ static uint16_t phys_section_add(MemoryRegionSection *section) return phys_sections_nb++; } +static void phys_section_destroy(MemoryRegion *mr) +{ + if (mr->subpage) { + subpage_t *subpage = container_of(mr, subpage_t, iomem); + memory_region_destroy(&subpage->iomem); + g_free(subpage); + } +} + static void phys_sections_clear(void) { - phys_sections_nb = 0; + while (phys_sections_nb > 0) { + MemoryRegionSection *section = &phys_sections[--phys_sections_nb]; + phys_section_destroy(section->mr); + } } static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section)
phys_sections_clear is invoked after the dispatch tree has been destroyed. This leaves a window where phys_sections_nb > 0 but the subpages are not valid anymore, which is a recipe for use-after-free bugs. Move the destruction of subpages in phys_sections_clear. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- exec.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)