| Message ID | 1497875079-8169-2-git-send-email-ppircalabu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@bitdefender.com> > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > --- > tools/libxc/include/xenctrl.h | 2 +- > tools/libxc/xc_monitor.c | 5 ++++- Acked-by: Wei Liu <wei.liu2@citrix.com>
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@bitdefender.com> > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> 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;
On Wed, Jun 21, 2017 at 7:58 AM, Wei Liu <wei.liu2@citrix.com> 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@bitdefender.com> >> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > > 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;
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@bitdefender.com> >> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > > 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
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@bitdefender.com> > >> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > > > > 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@lists.xen.org > https://lists.xen.org/xen-devel
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, &domctl); } 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; + /* Allignment padding */ + uint8_t pad1; + uint32_t pad2; + /* + * Send event only if the changed bit in the control register + * is not masked. + */ + uint64_aligned_t bitmask; } mov_to_cr; struct {