Re: [Xen-devel] [PATCH 2/2] xen/x86: hap: Clean-up and harden hap_enable()

[Julien Grall <julien@xxxxxxx>]

On 04/02/2020 10:51, Roger Pau Monné wrote:
> On Tue, Feb 04, 2020 at 09:34:11AM +0000, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > Unlike shadow_enable(), hap_enable() can only be called once during
> > domain creation and with the mode equal to mode equal to
>                                      ^ equals to
Will fix it.

> > PG_external | PG_translate | PG_refcounts.
> >
> > If it were called twice, then we might have something interesting
>                                                ^ a problem
> > problem as the p2m tables would be re-allocated (and therefore all the
> > mappings would be lost).
> >
> > Add code to sanity check the mode and that the function is only called
> > once. Take the opportunity to an if checking that PG_translate is set.
>                                  ^ add an if
Will fix it.

> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > ---
> >
> > It is not entirely clear when PG_translate was enforced.
> > ---
> >   xen/arch/x86/mm/hap/hap.c | 18 +++++++++++-------
> >   1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> > index 31362a31b6..b734e2e6d3 100644
> > --- a/xen/arch/x86/mm/hap/hap.c
> > +++ b/xen/arch/x86/mm/hap/hap.c
> > @@ -445,6 +445,13 @@ int hap_enable(struct domain *d, u32 mode)
> >       unsigned int i;
> >       int rv = 0;
> >   
> > +    if ( mode != (PG_external | PG_translate | PG_refcounts) )
> > +        return -EINVAL;
> > +
> > +    /* The function can only be called once */
> > +    if ( d->arch.paging.mode != 0 )
> > +        return -EINVAL;
> If you want to return EINVAL for both they can be merged into a single
> if. Also note that this would usually be written as
> if ( d->arch.paging.mode ) to keep it shorter.
To be honest, this is a matter of taste. There is also an argument that 
for MISRA, your suggestion is not compliant (see Rule 14.4).
> Albeit I think you might want to return EEXIST instead of EINVAL if
> mode is already set.
I am happy with that.

> > +
> >       domain_pause(d);
> >   
> >       old_pages = d->arch.paging.hap.total_pages;
> > @@ -465,13 +472,10 @@ int hap_enable(struct domain *d, u32 mode)
> >       d->arch.paging.alloc_page = hap_alloc_p2m_page;
> >       d->arch.paging.free_page = hap_free_p2m_page;
> >   
> > -    /* allocate P2m table */
> > -    if ( mode & PG_translate )
> > -    {
> > -        rv = p2m_alloc_table(p2m_get_hostp2m(d));
> > -        if ( rv != 0 )
> > -            goto out;
> > -    }
> > +    /* allocate P2M table */
> > +    rv = p2m_alloc_table(p2m_get_hostp2m(d));
> > +    if ( rv != 0 )
> I would also avoid comparing against 0 here.
See above.

Cheers,

--
Julien Grall

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