On 16.12.2022 12:48, Julien Grall wrote:
> From: Hongyan Xia <hongy...@amazon.com>
>
> When there is not an always-mapped direct map, xenheap allocations need
> to be mapped and unmapped on-demand.
Hmm, that's still putting mappings in the directmap, which I thought we
mean to be doing away with. If that's just a temporary step, then please
say so here.
> I have left the call to map_pages_to_xen() and destroy_xen_mappings()
> in the split heap for now. I am not entirely convinced this is necessary
> because in that setup only the xenheap would be always mapped and
> this doesn't contain any guest memory (aside the grant-table).
> So map/unmapping for every allocation seems unnecessary.
But if you're unmapping, that heap won't be "always mapped" anymore. So
why would it need mapping initially?
> Changes since Hongyan's version:
> * Rebase
> * Fix indentation in alloc_xenheap_pages()
Looks like you did in one of the two instances only, as ...
> @@ -2230,17 +2231,36 @@ void *alloc_xenheap_pages(unsigned int order,
> unsigned int memflags)
> if ( unlikely(pg == NULL) )
> return NULL;
>
> + ret = page_to_virt(pg);
> +
> + if ( !arch_has_directmap() &&
> + map_pages_to_xen((unsigned long)ret, page_to_mfn(pg), 1UL << order,
> + PAGE_HYPERVISOR) )
> + {
> + /* Failed to map xenheap pages. */
> + free_heap_pages(pg, order, false);
> + return NULL;
> + }
... this looks wrong.
An important aspect here is that to be sure of no recursion,
map_pages_to_xen() and destroy_xen_mappings() may no longer use Xen
heap pages. May be worth saying explicitly in the description (I can't
think of a good place in code where such a comment could be put _and_
be likely to be noticed at the right point in time).
> void free_xenheap_pages(void *v, unsigned int order)
> {
> + unsigned long va = (unsigned long)v & PAGE_MASK;
> +
> ASSERT_ALLOC_CONTEXT();
>
> if ( v == NULL )
> return;
>
> + if ( !arch_has_directmap() &&
> + destroy_xen_mappings(va, va + (1UL << (order + PAGE_SHIFT))) )
> + dprintk(XENLOG_WARNING,
> + "Error while destroying xenheap mappings at %p, order %u\n",
> + v, order);
Doesn't failure here mean (intended) security henceforth isn't guaranteed
anymore? If so, a mere dprintk() can't really be sufficient to "handle"
the error.
Jan
