Re: [Xen-devel] [PATCH v3 2/3] xen/pvh: check permissions when adding MMIO regions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.01.15 at 12:29, <roger.pau@xxxxxxxxxx> wrote:
> El 22/01/15 a les 16.43, Jan Beulich ha escrit:
>>>>> On 22.01.15 at 16:19, <roger.pau@xxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/domain_build.c
>>> +++ b/xen/arch/x86/domain_build.c
>>> @@ -320,11 +320,24 @@ static __init void pvh_add_mem_mapping(struct domain 
> *d, unsigned long gfn,
>>>  {
>>>      unsigned long i;
>>>      p2m_access_t a;
>>> +    mfn_t omfn;
>>> +    p2m_type_t t;
>>>      int rc;
>>>  
>>> -    a = p2m_get_hostp2m(d)->default_access;
>> 
>> Iirc this is rwx.
>> 
>>>      for ( i = 0; i < nr_mfns; i++ )
>>>      {
>>> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
>>> +        {
>>> +            omfn = get_gfn_query_unlocked(d, gfn + i, &t);
>>> +            guest_physmap_remove_page(d, gfn + i, mfn_x(omfn), 
> PAGE_ORDER_4K);
>>> +            continue;
>>> +        }
>>> +
>>> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) )
>>> +            a = p2m_access_r;
>>> +        else
>>> +            a = p2m_access_rw;
>> 
>> Shouldn't these two therefore be rx and rwx respectively? Or even
>> better ->default_access in the else case (albeit that doesn't really
>> matter here since nothing can have changed that field from its
>> default value)? I'm particularly thinking of ROMs that may be sitting
>> in these areas.
> 
> Yes, it should be rx and rwx. I would prefer to use the explicit types.
> IMHO it's easier to understand to which access type it's set (without
> having to dig to what value p2m_get_hostp2m(d)->default_access is set).

That's fine then.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel