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

["gao, chao" <chao.gao@xxxxxxxxx>]

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@xxxxxxxxx>
>
>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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel