[PATCH] x86/vMsi-x: check whether the msixtbl_list has been initialized or not when accessing it

[PATCH] x86/vMsi-x: check whether the msixtbl_list has been initialized or not when accessing it

[Chao Gao]

MSI-x tables' initialization had been detered in the commit
74c6dc2d0ac4dcab0c6243cdf6ed550c1532b798. If an assigned device does not support
MSI-x, the msixtbl_list won't be initialized. Howerver, both of following paths
    XEN_DOMCTL_bind_pt_irq
        pt_irq_create_bind
            msixtbl_pt_register
and
    XEN_DOMCTL_unbind_pt_irq
        pt_irq_destroy_bind
            msixtbl_pt_unregister
do not check this case and will cause Xen panic consequently.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 xen/arch/x86/hvm/vmsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index e418b98..e0d710b 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -449,7 +449,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
     ASSERT(pcidevs_locked());
     ASSERT(spin_is_locked(&d->event_lock));
 
-    if ( !has_vlapic(d) )
+    if ( !has_vlapic(d) || !d->arch.hvm_domain.msixtbl_list.next )
         return -ENODEV;
 
     /*
@@ -519,7 +519,7 @@ void msixtbl_pt_unregister(struct domain *d, struct pirq *pirq)
     ASSERT(pcidevs_locked());
     ASSERT(spin_is_locked(&d->event_lock));
 
-    if ( !has_vlapic(d) )
+    if ( !has_vlapic(d) || !d->arch.hvm_domain.msixtbl_list.next )
         return;
 
     irq_desc = pirq_spin_lock_irq_desc(pirq, NULL);
-- 
1.8.3.1


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

Re: [PATCH] x86/vMsi-x: check whether the msixtbl_list has been initialized or not when accessing it

[Andrew Cooper]

On 29/07/16 02:35, Chao Gao wrote:
> MSI-x tables' initialization had been detered in the commit
> 74c6dc2d0ac4dcab0c6243cdf6ed550c1532b798. If an assigned device does not support
> MSI-x, the msixtbl_list won't be initialized. Howerver, both of following paths
>     XEN_DOMCTL_bind_pt_irq
>         pt_irq_create_bind
>             msixtbl_pt_register
> and
>     XEN_DOMCTL_unbind_pt_irq
>         pt_irq_destroy_bind
>             msixtbl_pt_unregister
> do not check this case and will cause Xen panic consequently.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>

This issue was already reported and I provided a fix in

https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=db0eee0a071e2e3e18e79d21a9b1d6724edeeeb3

However, looking at your patch, I forgot to fix the
msixtbl_pt_register() path, so your patch is still necessary.

Please rebase this patch onto the staging branch which has the
aformentioned fix in, at which point it can be accepted.  Just one note.

> ---
>  xen/arch/x86/hvm/vmsi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index e418b98..e0d710b 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -449,7 +449,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
>      ASSERT(pcidevs_locked());
>      ASSERT(spin_is_locked(&d->event_lock));
>  
> -    if ( !has_vlapic(d) )
> +    if ( !has_vlapic(d) || !d->arch.hvm_domain.msixtbl_list.next )

You can drop the vlapic() check, as it is redundant with whether msixtbl
is enabled or not.

~Andrew

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

Re: [PATCH] x86/vMsi-x: check whether the msixtbl_list has been initialized or not when accessing it

[gao, chao]

On Fri, Jul 29, 2016 at 10:30:07AM +0100, Andrew Cooper wrote:
>On 29/07/16 02:35, Chao Gao wrote:
>> MSI-x tables' initialization had been detered in the commit
>> 74c6dc2d0ac4dcab0c6243cdf6ed550c1532b798. If an assigned device does not support
>> MSI-x, the msixtbl_list won't be initialized. Howerver, both of following paths
>>     XEN_DOMCTL_bind_pt_irq
>>         pt_irq_create_bind
>>             msixtbl_pt_register
>> and
>>     XEN_DOMCTL_unbind_pt_irq
>>         pt_irq_destroy_bind
>>             msixtbl_pt_unregister
>> do not check this case and will cause Xen panic consequently.
>>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>
>This issue was already reported and I provided a fix in
>
>https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=db0eee0a071e2e3e18e79d21a9b1d6724edeeeb3

I'm sorry for the mistake.

>However, looking at your patch, I forgot to fix the
>msixtbl_pt_register() path, so your patch is still necessary.

Actually, the msixtbl_pt_register() path never causes a panic unless wrong hypercall
paramters are given. Specially, we assign a msi capable but not msi-x capable device
to guest, but some errors(malwares, etc.) lead to calling XEN_DOMCTL_bind_pt_irq 
without a clear gtable.
>Please rebase this patch onto the staging branch which has the
>aformentioned fix in, at which point it can be accepted.  Just one note.

Thanks for your advice.
>> ---
>>  xen/arch/x86/hvm/vmsi.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
>> index e418b98..e0d710b 100644
>> --- a/xen/arch/x86/hvm/vmsi.c
>> +++ b/xen/arch/x86/hvm/vmsi.c
>> @@ -449,7 +449,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
>>      ASSERT(pcidevs_locked());
>>      ASSERT(spin_is_locked(&d->event_lock));
>>  
>> -    if ( !has_vlapic(d) )
>> +    if ( !has_vlapic(d) || !d->arch.hvm_domain.msixtbl_list.next )
>
>You can drop the vlapic() check, as it is redundant with whether msixtbl
>is enabled or not.
>
>~Andrew

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