Re: [Xen-devel] [PATCH for-4.12 v2 05/17] xen/arm: p2m: Handle translation fault in get_page_from_gva

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 7 Dec 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/12/2018 22:04, Stefano Stabellini wrote:
> > On Wed, 5 Dec 2018, Julien Grall wrote:
> > > On 04/12/2018 23:59, Stefano Stabellini wrote:
> > > > On Tue, 4 Dec 2018, Julien Grall wrote:
> > > > > A follow-up patch will re-purpose the valid bit of LPAE entries to
> > > > > generate fault even on entry containing valid information.
> > > > > 
> > > > > This means that when translating a guest VA to guest PA (e.g IPA) will
> > > > > fail if the Stage-2 entries used have the valid bit unset. Because of
> > > > > that, we need to fallback to walk the page-table in software to check
> > > > > whether the fault was expected.
> > > > > 
> > > > > This patch adds the software page-table walk on all the translation
> > > > > fault. It would be possible in the future to avoid pointless walk when
> > > > > the fault in PAR_EL1 is not a translation fault.
> > > > > 
> > > > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > > > 
> > > > > ---
> > > > > 
> > > > > There are a couple of TODO in the code. They are clean-up and
> > > > > performance
> > > > > improvement (e.g when the fault cannot be handled) that could be
> > > > > delayed
> > > > > after
> > > > > the series has been merged.
> > > > > 
> > > > >       Changes in v2:
> > > > >           - Check stage-2 permission during software lookup
> > > > >           - Fix typoes
> > > > > ---
> > > > >    xen/arch/arm/p2m.c | 66
> > > > > ++++++++++++++++++++++++++++++++++++++++++++++++------
> > > > >    1 file changed, 59 insertions(+),should  7 deletions(-)
> > > > > 
> > > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > > index 47b54c792e..39680eeb6e 100644
> > > > > --- a/xen/arch/arm/p2m.c
> > > > > +++ b/xen/arch/arm/p2m.c
> > > > > @@ -6,6 +6,7 @@
> > > > >      #include <asm/event.h>
> > > > >    #include <asm/flushtlb.h>
> > > > > +#include <asm/guest_walk.h>
> > > > >    #include <asm/page.h>
> > > > >      #define MAX_VMID_8_BIT  (1UL << 8)
> > > > > @@ -1430,6 +1431,8 @@ struct page_info *get_page_from_gva(struct vcpu
> > > > > *v,
> > > > > vaddr_t va,
> > > > >        struct page_info *page = NULL;
> > > > >        paddr_t maddr = 0;
> > > > >        uint64_t par;
> > > > > +    mfn_t mfn;
> > > > > +    p2m_type_t t;
> > > > >          /*
> > > > >         * XXX: To support a different vCPU, we would need to load the
> > > > > @@ -1446,8 +1449,29 @@ struct page_info *get_page_from_gva(struct vcpu
> > > > > *v,
> > > > > vaddr_t va,
> > > > >        par = gvirt_to_maddr(va, &maddr, flags);
> > > > >        p2m_read_unlock(p2m);
> > > > >    +    /*
> > > > > +     * gvirt_to_maddr may fail if the entry does not have the valid
> > > > > bit
> > > > > +     * set. Fallback to the second method:
> > > > > +     *  1) Translate the VA to IPA using software lookup -> Stage-1
> > > > > page-table
> > > > > +     *  may not be accessible because the stage-2 entries may have
> > > > > valid
> > > > > +     *  bit unset.
> > > > > +     *  2) Software lookup of the MFN
> > > > > +     *
> > > > > +     * Note that when memaccess is enabled, we instead call directly
> > > > > +     * p2m_mem_access_check_and_get_page(...). Because the function
> > > > > is a
> > > > > +     * a variant of the methods described above, it will be able to
> > > > > +     * handle entries with valid bit unset.
> > > > > +     *
> > > > > +     * TODO: Integrate more nicely memaccess with the rest of the
> > > > > +     * function.
> > > > > +     * TODO: Use the fault error in PAR_EL1 to avoid pointless
> > > > > +     *  translation.
> > > > > +     */
> > > > >        if ( par )
> > > > >        {
> > > > > +        paddr_t ipa;
> > > > > +        unsigned int s1_perms;
> > > > > +
> > > > >            /*
> > > > >             * When memaccess is enabled, the translation GVA to MADDR
> > > > > may
> > > > >             * have failed because of a permission fault.
> > > > > @@ -1455,20 +1479,48 @@ struct page_info *get_page_from_gva(struct
> > > > > vcpu
> > > > > *v, vaddr_t va,
> > > > >            if ( p2m->mem_access_enabled )
> > > > >                return p2m_mem_access_check_and_get_page(va, flags, v);
> > > > >    -        dprintk(XENLOG_G_DEBUG,
> > > > > -                "%pv: gvirt_to_maddr failed va=%#"PRIvaddr"
> > > > > flags=0x%lx
> > > > > par=%#"PRIx64"\n",
> > > > > -                v, va, flags, par);
> > > > > -        return NULL;
> > > > > +        /*
> > > > > +         * The software stage-1 table walk can still fail, e.g, if
> > > > > the
> > > > > +         * GVA is not mapped.
> > > > > +         */
> > > > > +        if ( !guest_walk_tables(v, va, &ipa, &s1_perms) )
> > > > > +        {
> > > > > +            dprintk(XENLOG_G_DEBUG,
> > > > > +                    "%pv: Failed to walk page-table va
> > > > > %#"PRIvaddr"\n",
> > > > > v, va);
> > > > > +            return NULL;
> > > > > +        }
> > > > > +
> > > > > +        mfn = p2m_lookup(d, gaddr_to_gfn(ipa), &t);
> > > > > +        if ( mfn_eq(INVALID_MFN, mfn) || !p2m_is_ram(t) )
> > > > > +            return NULL;
> > > > > +
> > > > > +        /*
> > > > > +         * Check permission that are assumed by the caller. For
> > > > > instance
> > > > > +         * in case of guestcopy, the caller assumes that the
> > > > > translated
> > > > > +         * page can be accessed with the requested permissions. If
> > > > > this
> > > > > +         * is not the case, we should fail.
> > > > > +         *
> > > > > +         * Please note that we do not check for the GV2M_EXEC
> > > > > +         * permission. This is fine because the hardware-based
> > > > > translation
> > > > > +         * instruction does not test for execute permissions.
> > > > > +         */
> > > > > +        if ( (flags & GV2M_WRITE) && !(s1_perms & GV2M_WRITE) )
> > > > > +            return NULL;
> > > > > +
> > > > > +        if ( (flags & GV2M_WRITE) && t != p2m_ram_rw )
> > > > > +            return NULL;
> > > > 
> > > > The patch looks good enough now. One question: is it a requirement that
> > > > the page we are trying to translate is of type p2m_ram_*? Could
> > > > get_page_from_gva be genuinely called passing a page of a different
> > > > kind, such as p2m_mmio_direct_* or p2m_map_foreign? Today, it is not the
> > > > case, but I wonder if it is something we want to consider?
> > > 
> > > This function can only possibly work with p2m_ram_* because of the
> > > get_page(...) below, indeed the page should belong to the domain.
> > > 
> > > Effectively this function will only be used for hypercall as you use a
> > > virtual
> > > address. I question the value of allowing a guest to do a hypercall with
> > > the
> > > data backed in any other memories than guest RAM. For the foreign mapping,
> > > this could potentially end up with a leakage.
> > 
> > OK.
> 
> I can probably add a few more dprintk in the error paths to help the developer
> to diagnostics the problem if that's ever happen. What do you think?

Maybe a short one line comment on top to say "this function only work
for guest ram pages (no foreign mappings or MMIO regions.)" just to make
it very obvious but it is not necessary, up to you


> > 
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> Thank you!
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel