Re: [Xen-devel] [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 
>
>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(>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: [Xen-devel] [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 

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(>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

[Xen-devel] [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 
---
 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(>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(>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