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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jun 21, 2017 at 05:13:29PM +0300, Razvan Cojocaru wrote:
> 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. :)
> 

OK. Please submit a patch to fix this. Using 3 should be fine. Please
include

  Coverity-ID: 1412966

in the commit message.

> 
> Thanks,
> Razvan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel

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