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

[Julien Grall <julien.grall@arm.com>]

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,

-- 
Julien Grall

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