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

2017-06-21 Thread Wei Liu 
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 
> >> Acked-by: Tamas K Lengyel 
> > 
> > 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
>

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

2017-06-21 Thread Razvan Cojocaru 
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 
>> Acked-by: Tamas K Lengyel 
> 
> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

2017-06-21 Thread Tamas K Lengyel 
On Wed, Jun 21, 2017 at 7:58 AM, 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 
>> Acked-by: Tamas K Lengyel 
>
> 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.

Indeed. We have a sanity check earlier in here that checks whether
index > 31 but it would make more sense to check it against the max
valid value of index to begin with (which at the moment is
VM_EVENT_X86_XCR0 = 3).

>
> ** 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;

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

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

2017-06-21 Thread Wei Liu 
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 
> Acked-by: Tamas K Lengyel 

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; 
 

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

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

2017-06-20 Thread Wei Liu 
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 
> Acked-by: Tamas K Lengyel 
> ---
>  tools/libxc/include/xenctrl.h | 2 +-
>  tools/libxc/xc_monitor.c  | 5 -

Acked-by: Wei Liu 

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

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

2017-06-19 Thread Petre Pircalabu 
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 
Acked-by: Tamas K Lengyel 
---
 tools/libxc/include/xenctrl.h | 2 +-
 tools/libxc/xc_monitor.c  | 5 -
 xen/arch/x86/hvm/monitor.c| 3 ++-
 xen/arch/x86/monitor.c| 9 +
 xen/include/asm-x86/domain.h  | 1 +
 xen/include/public/domctl.h   | 8 
 6 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 1629f41..8c26cb4 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1999,7 +1999,7 @@ int xc_monitor_get_capabilities(xc_interface *xch, 
domid_t domain_id,
 uint32_t *capabilities);
 int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
  uint16_t index, bool enable, bool sync,
- bool onchangeonly);
+ uint64_t bitmask, bool onchangeonly);
 /*
  * A list of MSR indices can usually be found in /usr/include/asm/msr-index.h.
  * Please consult the Intel/AMD manuals for more information on
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index f99b6e3..b44ce93 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -70,7 +70,7 @@ int xc_monitor_get_capabilities(xc_interface *xch, domid_t 
domain_id,
 
 int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
  uint16_t index, bool enable, bool sync,
- bool onchangeonly)
+ uint64_t bitmask, bool onchangeonly)
 {
 DECLARE_DOMCTL;
 
@@ -82,6 +82,9 @@ int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t 
domain_id,
 domctl.u.monitor_op.u.mov_to_cr.index = index;
 domctl.u.monitor_op.u.mov_to_cr.sync = sync;
 domctl.u.monitor_op.u.mov_to_cr.onchangeonly = onchangeonly;
+domctl.u.monitor_op.u.mov_to_cr.bitmask = bitmask;
+domctl.u.monitor_op.u.mov_to_cr.pad1 = 0;
+domctl.u.monitor_op.u.mov_to_cr.pad2 = 0;
 
 return do_domctl(xch, );
 }
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index bde5fd0..a7ccfc4 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -38,7 +38,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long 
value, unsigned long old
 
 if ( (ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) &&
  (!(ad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) ||
-  value != old) )
+  value != old) &&
+ (!((value ^ old) & ad->monitor.write_ctrlreg_mask[index])) )
 {
 bool_t sync = !!(ad->monitor.write_ctrlreg_sync & ctrlreg_bitmask);
 
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 449c64c..bedf13c 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -136,6 +136,9 @@ int arch_monitor_domctl_event(struct domain *d,
 if ( unlikely(mop->u.mov_to_cr.index > 31) )
 return -EINVAL;
 
+if ( unlikely(mop->u.mov_to_cr.pad1 || mop->u.mov_to_cr.pad2) )
+return -EINVAL;
+
 ctrlreg_bitmask = monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
 old_status = !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask);
 
@@ -155,9 +158,15 @@ int arch_monitor_domctl_event(struct domain *d,
 ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
 
 if ( requested_status )
+{
+ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = 
mop->u.mov_to_cr.bitmask;
 ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;
+}
 else
+{
+ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = 0;
 ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
+}
 
 if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )
 {
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 924caac..27d80ee 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -406,6 +406,7 @@ struct arch_domain
 unsigned int cpuid_enabled   : 1;
 unsigned int descriptor_access_enabled   : 1;
 struct monitor_msr_bitmap *msr_bitmap;
+uint64_t write_ctrlreg_mask[4];
 } monitor;
 
 /* Mem_access emulation control */
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index f7cbc0a..ff39762 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1107,6 +1107,14 @@ struct xen_domctl_monitor_op {
 uint8_t sync;
 /* Send event only on a change of value */
 uint8_t onchangeonly;
+/*