RE: [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, September 6, 2022 2:34 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
> Julien Grall <julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> Wei Liu <wl@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v10 8/9] xen: retrieve reserved pages on
> populate_physmap
> 
> On 05.09.2022 09:08, Penny Zheng wrote:
> > Hi jan
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Wednesday, August 17, 2022 6:05 PM
> >> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> >> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Andrew Cooper
> >> <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> >> <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano
> >> Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>;
> >> xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH v10 8/9] xen: retrieve reserved pages on
> >> populate_physmap
> >>
> >> On 16.08.2022 04:36, Penny Zheng wrote:
> >>> @@ -2867,6 +2854,61 @@ int __init acquire_domstatic_pages(struct
> >>> domain *d, mfn_t smfn,
> >>>
> >>>      return 0;
> >>>  }
> >>> +
> >>> +/*
> >>> + * Acquire nr_mfns contiguous pages, starting at #smfn, of static
> >>> +memory,
> >>> + * then assign them to one specific domain #d.
> >>> + */
> >>> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> >>> +                                   unsigned int nr_mfns, unsigned
> >>> +int
> >>> +memflags) {
> >>> +    struct page_info *pg;
> >>> +
> >>> +    ASSERT_ALLOC_CONTEXT();
> >>> +
> >>> +    pg = acquire_staticmem_pages(smfn, nr_mfns, memflags);
> >>> +    if ( !pg )
> >>> +        return -ENOENT;
> >>> +
> >>> +    if ( assign_domstatic_pages(d, pg, nr_mfns, memflags) )
> >>> +        return -EINVAL;
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Acquire a page from reserved page list(resv_page_list), when
> >>> +populating
> >>> + * memory for static domain on runtime.
> >>> + */
> >>> +mfn_t acquire_reserved_page(struct domain *d, unsigned int
> >>> +memflags) {
> >>> +    struct page_info *page;
> >>> +
> >>> +    ASSERT_ALLOC_CONTEXT();
> >>> +
> >>> +    /* Acquire a page from reserved page list(resv_page_list). */
> >>> +    spin_lock(&d->page_alloc_lock);
> >>> +    page = page_list_remove_head(&d->resv_page_list);
> >>> +    spin_unlock(&d->page_alloc_lock);
> >>> +    if ( unlikely(!page) )
> >>> +        return INVALID_MFN;
> >>> +
> >>> +    if ( !prepare_staticmem_pages(page, 1, memflags) )
> >>> +        goto fail;
> >>> +
> >>> +    if ( assign_domstatic_pages(d, page, 1, memflags) )
> >>> +        goto fail_assign;
> >>> +
> >>> +    return page_to_mfn(page);
> >>> +
> >>> + fail_assign:
> >>> +    free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
> >>
> >> Doesn't this need to be !(memflags & MEMF_no_scrub)? And then - with
> >
> > I got a bit confused about this flag MEMF_no_scrub, does it mean no
> > need to scrub?
> 
> Yes, as its name says.
> 
> > Since I saw that in alloc_domheap_pages(...)
> >     if ( assign_page(pg, order, d, memflags) )
> >     {
> >         free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> >         return NULL;
> >     }
> > It doesn't contain exclamation mark too...
> 
> Hmm, you're right - on these error paths the scrubbing is needed if the page
> wasn't previously scrubbed, as part of the set of pages may have been
> transiently exposed to the guest (and by guessing it may have been able to
> actually access the pages; I'm inclined to say it's its own fault though if 
> that
> way information is being leaked).
> 

Then, the same for the acquire_domstatic_pages(...)

    if ( assign_pages(pg, nr_mfns, d, memflags) )
    {
        free_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub);
        return -EINVAL;
    }
On this error path, it has misused the MEMF_no_scrub too.
But IMO, as we are talking about these pages will always be reserved to the 
guest,
maybe here it also doesn't need scrubbing at all?
 
> But ...
> 
> >> assignment having failed and with it being just a single page we're
> >> talking about, the page was not exposed to the guest at any point
> >> afaict. So I don't see the need for scrubbing in the first place.
> 
> while my comment wasn't really correct, as said - you don't need any
> scrubbing here at all, I think.
> 
> Jan