[Xen-devel] Ping: [PATCH v3 5/5] x86/MSI: drop workaround for insecure Dom0 kernels

["Jan Beulich" <JBeulich@xxxxxxxx>]

>> Considering that
>> - the workaround is expensive (iterating through the entire P2M space
>>   of a domain),
>> - the planned elimination of the expensiveness (by propagating the type
>>   change step by step to the individual P2M leaves) wouldn't address
>>   the IOMMU side of things (as for it to obey to the changed
>>   permissions the adjustments must be pushed down immediately through
>>   the entire tree)
>> - the proper solution (PHYSDEVOP_msix_prepare) should by now be
>>   implemented by all security conscious Dom0 kernels
>> remove the workaround, killing eventual guests that would be known to
>> become a security risk instead.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> 
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -770,7 +770,7 @@ static void ept_change_entry_type_global
>>          return;
>> 
>>      BUG_ON(p2m_is_grant(ot) || p2m_is_grant(nt));
>> -    BUG_ON(ot != nt && (ot == p2m_mmio_direct || nt ==
>> p2m_mmio_direct));
>> +    BUG_ON(p2m_is_mmio(ot) || p2m_is_mmio(nt));
>> 
>>      ept_change_entry_type_page(_mfn(ept_get_asr(ept)),
>>                                 ept_get_wl(ept), ot, nt);
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -825,32 +825,22 @@ static int msix_capability_init(struct p
>>                                  msix->pba.last) )
>>              WARN();
>> 
>> -        if ( dev->domain )
>> -            p2m_change_entry_type_global(dev->domain,
>> -                                         p2m_mmio_direct,
>> p2m_mmio_direct);
>> -        if ( desc && (!dev->domain
>> || !paging_mode_translate(dev->domain)) )
>> +        if ( desc )
>>          {
>> -            struct domain *d = dev->domain;
>> +            struct domain *currd = current->domain;
>> +            struct domain *d = dev->domain ?: currd;
>> 
>> -            if ( !d )
>> -                for_each_domain(d)
>> -                    if ( !paging_mode_translate(d) &&
>> -                         (iomem_access_permitted(d, msix->table.first,
>> -                                                 msix->table.last)
>> ||
>> -                          iomem_access_permitted(d, msix->pba.first,
>> -                                                 msix->pba.last)) )
>> -                        break;
>> -            if ( d )
>> -            {
>> -                if ( !is_hardware_domain(d) && msix->warned !=
>> d->domain_id )
>> -                {
>> -                    msix->warned = d->domain_id;
>> -                    printk(XENLOG_ERR
>> -                           "Potentially insecure use of MSI-X
>> on %04x:%02x:%02x.%u by Dom%d\n",
>> -                           seg, bus, slot, func, d->domain_id);
>> -                }
>> -                /* XXX How to deal with existing mappings? */
>> -            }
>> +            if ( !is_hardware_domain(currd) || d != currd )
>> +                printk("%s use of MSI-X on %04x:%02x:%02x.%u by
>> Dom%d\n",
>> +                       is_hardware_domain(currd)
>> +                       ? XENLOG_WARNING "Potentially insecure"
>> +                       : XENLOG_ERR "Insecure",
>> +                       seg, bus, slot, func, d->domain_id);
>> +            if ( !is_hardware_domain(d) &&
>> +                 /* Assume a domain without memory has no mappings
>> yet. */
>> +                 (!is_hardware_domain(currd) || d->tot_pages) )
>> +                domain_crash(d);
>> +            /* XXX How to deal with existing mappings? */
>>          }
>>      }
>>      WARN_ON(msix->nr_entries != nr_entries);
>> 
>> 




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