Re: [Xen-devel] [PATCH v4 1/2] x86/monitor: add masking support for write_ctrlreg events

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 06/21/2017 04:58 PM, Wei Liu wrote:
> On Mon, Jun 19, 2017 at 03:24:38PM +0300, Petre Pircalabu wrote:
>> Add support for filtering out the write_ctrlreg monitor events if they
>> are generated only by changing certains bits.
>> A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg
>> function in order to mask the event generation if the changed bits are
>> set.
>>
>> Signed-off-by: Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>
>> Acked-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> 
> Coverity isn't happy with this patch.
> 
> It seems to me there is indeed a risk to overrun the buffer (4 in size) 
> because
> the caller can specify index up to 31.
> 
> ** CID 1412966:  Memory - corruptions  (OVERRUN)                              
>                                                                               
>   
> /xen/arch/x86/monitor.c: 162 in arch_monitor_domctl_event()                   
>                                                                               
>   
>                                                                               
>                                                                               
>   
>                                                                               
>                                                                               
>   
> ________________________________________________________________________________________________________
>                                                       
> *** CID 1412966:  Memory - corruptions  (OVERRUN)                             
>                                                                               
>   
> /xen/arch/x86/monitor.c: 162 in arch_monitor_domctl_event()                   
>                                                                               
>   
> 156                 ad->monitor.write_ctrlreg_onchangeonly |= 
> ctrlreg_bitmask;                                                              
>                   
> 157             else                                                          
>                                                                               
>   
> 158                 ad->monitor.write_ctrlreg_onchangeonly &= 
> ~ctrlreg_bitmask;                                                             
>                   
> 159                                                                           
>                                                                               
>   
> 160             if ( requested_status )                                       
>                                                                               
>   
> 161             {                                                             
>                                                                               
>   
>>>>     CID 1412966:  Memory - corruptions  (OVERRUN)                          
>>>>                                                                            
>>>>     
>>>>     Overrunning array "ad->monitor.write_ctrlreg_mask" of 4 8-byte 
>>>> elements at element index 31 (byte offset 248) using index 
>>>> "mop->u.mov_to_cr.index"    
> (which evaluates to 31).                                                      
>                                                                               
>   
> 162                 ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = 
> mop->u.mov_to_cr.bitmask;                                                     
>    
> 163                 ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;     
>                                                                               
>   
> 164             }                                                             
>                                                                               
>   
> 165             else                                                          
>                                                                               
>   
> 166             {                                                             
>                                                                               
>   
> 167                 ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = 
> 0;      

I vaguely remember that 31 was introduced simply as a "reserved"
precaution - we can probably safely please Coverity by simply patching
that code to not go over 3 as an index.

To Petre's credit, he did notice and propose that we change this value
but I've suggested that we keep the check as-is for the future. My bad. :)


Thanks,
Razvan


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