| Message ID | 20190812222844.9636-6-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/7] xen/arm: pass node to device_tree_for_each_node | expand |
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@xilinx.com> > --- > > 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(). But, if you want to find "/reserved-memory" anywhere in dt_node_full_name(dev), then you probably want to use strcasestr() > { > - printk(XENLOG_ERR "Unable to permit to dom%d access to" > - " 0x%"PRIx64" - 0x%"PRIx64"\n", > - d->domain_id, > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > - return res; > + res = iomem_permit_access(d, paddr_to_pfn(addr), > + paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); > + if ( res ) > + { > + printk(XENLOG_ERR "Unable to permit to dom%d access to" > + " 0x%"PRIx64" - 0x%"PRIx64"\n", > + d->domain_id, > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > + return res; > + } > } > > if ( need_mapping )
Hi, 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@xilinx.com> >> --- >> >> 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. On a side note, the check is still incorrect here because you would allow /reserved-memory@... or /reserved-memory-test to pass. Cheers,
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@xilinx.com> > > > --- > > > > > > 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.
Hi Stefano, On 14/08/2019 23:40, Stefano Stabellini wrote: > On Tue, 13 Aug 2019, Julien Grall wrote: >> On 8/13/19 3:34 PM, Volodymyr Babchuk wrote: >>> Stefano Stabellini writes: >> 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. You are right, the trailing / should ensure you only match anything under the reserved-memory node. Cheers,
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 ) { - printk(XENLOG_ERR "Unable to permit to dom%d access to" - " 0x%"PRIx64" - 0x%"PRIx64"\n", - d->domain_id, - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); - return res; + res = iomem_permit_access(d, paddr_to_pfn(addr), + paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); + if ( res ) + { + printk(XENLOG_ERR "Unable to permit to dom%d access to" + " 0x%"PRIx64" - 0x%"PRIx64"\n", + d->domain_id, + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); + return res; + } } if ( need_mapping )
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@xilinx.com> --- 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(-)