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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich
> Sent: Monday, April 07, 2014 6:12 PM
> 
> 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.
> 

above looks reasonable to me... but I'm not familiar with original security
issue on MSI. Could you give me a link to previous conversation?

Thanks
Kevin

> 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