Re: [Xen-devel] [PATCH v1 for-4.6 1/2] xen/pvh: check permissions when adding MMIO regions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.12.14 at 20:05, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 18/12/14 18:27, Roger Pau Monne wrote:
>> --- a/xen/arch/x86/domain_build.c
>> +++ b/xen/arch/x86/domain_build.c
>> @@ -312,16 +312,47 @@ static __init void pvh_add_mem_mapping(struct domain 
>> *d, unsigned long gfn,
>>                                         unsigned long mfn, unsigned long 
>> nr_mfns)
>>  {
>>      unsigned long i;
>> +    xenmem_access_t def_access;
>> +    bool_t r_only = false;
>>      int rc;
>>  
>>      for ( i = 0; i < nr_mfns; i++ )
>>      {
>> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
>> +            goto next;
>> +
>> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) && 
>> !r_only )
>> +        {
>> +            rc = p2m_get_mem_access(d, ~0ul, &def_access);
>> +            BUG_ON(rc);
>> +            /* Set default access to read-only */
>> +            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, XENMEM_access_r);
>> +            BUG_ON(rc);
>> +            r_only = true;
>> +        }
>> +        else if ( r_only )
>> +        {
>> +            /* Set the default back */
>> +            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, def_access);
>> +            BUG_ON(rc);
>> +            r_only = false;
>> +        }
> 
> Am I missing something obvious, or would all this r_only juggling be far
> more easy if set_mmio_p2m_entry() had a ro/rw boolean parameter?
> 
> As these entries are done one at a time, it would seem to be far less
> error prone to explicitly choose a read-only or read-write mmio mapping,
> rather than playing with the entire default.

+1

>> +
>>          if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i))) )
>>              panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
>>                    gfn, mfn, i, rc);
>> +
>> + next:
>>          if ( !(i & 0xfffff) )
>>                  process_pending_softirqs();
> 
> You could remove the next label by moving this clause to the top and
> adding an i != 0 check.

Or really just make the respective goto a continue - we know we
don't hide overly large regions from Dom0.

Jan


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