Re: [Xen-devel] [PATCH v5 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 13 Aug 2019, Julien Grall wrote:
> On 8/13/19 3:34 PM, Volodymyr Babchuk wrote:
> > 
> > Stefano Stabellini writes:
> > 
> > > Don't allow reserved-memory regions to be remapped into any unprivileged
> > > guests, until reserved-memory regions are properly supported in Xen. For
> > > now, do not call iomem_permit_access on them, because giving
> > > iomem_permit_access to dom0 means that the toolstack will be able to
> > > assign the region to a domU.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > ---
> > > 
> > > Changes in v5:
> > > - fix check condition
> > > - use strnicmp
> > > - return error
> > > - improve commit message
> > > 
> > > Changes in v4:
> > > - compare the parent name with reserved-memory
> > > - use dt_node_cmp
> > > 
> > > Changes in v3:
> > > - new patch
> > > ---
> > >   xen/arch/arm/domain_build.c | 24 ++++++++++++++++--------
> > >   1 file changed, 16 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 4c8404155a..e0c0c01c88 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -1155,15 +1155,23 @@ static int __init map_range_to_domain(const struct
> > > dt_device_node *dev,
> > >       bool need_mapping = !dt_device_for_passthrough(dev);
> > >       int res;
> > >   -    res = iomem_permit_access(d, paddr_to_pfn(addr),
> > > -                              paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> > > -    if ( res )
> > > +    /*
> > > +     * Don't give iomem permissions for reserved-memory ranges to domUs
> > > +     * until reserved-memory support is complete.
> > > +     */
> > > +    if ( strnicmp(dt_node_full_name(dev), "/reserved-memory",
> > > +         strlen("/reserved-memory")) != 0 )
> > Why are you using strnicmp there? With such usage it is the same as
> > strcasecmp().
> 
> Definitely not, strcasecmp() will compare that the two strings exactly match
> (ignoring the case of the characters). Here we only want to check the first
> part of the patch, so we need to use the length-limited version.
> 
> > But, if you want to find "/reserved-memory" anywhere in
> > dt_node_full_name(dev), then you probably want to use strcasestr()
> 
> For a first strcasestr() is not provided by the string lib in Xen. So you
> would need to implement it.
> 
> But then all the reserved-memory regions are under the node /reserved-memory.
> A path /a/b/reserved-memory is not a valid memory region.

Yes, I think strnicmp is the right one but...


> On a side note, the check is still incorrect here because you would allow
> /reserved-memory@... or /reserved-memory-test to pass.

...but you are right that we should deal with things like
"/reserved-memory-test" properly. It looks like we should compare
against "/reserved-memory/", note the '/' at the end.

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