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 <pbonz...@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)
>
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
