On Tue, Jun 02, 2020 at 06:40:12AM -0600, Tamas K Lengyel wrote: > On Tue, Jun 2, 2020 at 5:08 AM Roger Pau Monné <roger....@citrix.com> wrote: > > > > On Wed, May 20, 2020 at 08:31:52PM -0600, Tamas K Lengyel wrote: > > > Extend the monitor_op domctl to include option that enables > > > controlling what values certain registers are permitted to hold > > > by a monitor subscriber. > > > > I think the change could benefit for some more detail commit message > > here. Why is this useful? > > You would have to ask the Bitdefender folks who made the feature. I > don't use it. Here we are just making it optional as it is buggy so it > is disabled by default. > > > > > There already seems to be some support for gating MSR writes, which > > seems to be expanded by this commit? > > We don't expand on any existing features, we make an existing feature > optional. > > > > > Is it solving some kind of bug reported? > > It does, please take a look at the cover letter.
Please copy the relevant bits here for reference. > > > > > Signed-off-by: Tamas K Lengyel <ta...@tklengyel.com> > > > --- > > > xen/arch/x86/hvm/hvm.c | 25 ++++++++++++++++--------- > > > xen/arch/x86/monitor.c | 10 +++++++++- > > > xen/include/asm-x86/domain.h | 1 + > > > xen/include/public/domctl.h | 1 + > > > 4 files changed, 27 insertions(+), 10 deletions(-) > > > > > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > > > index 09ee299bc7..e6780c685b 100644 > > > --- a/xen/arch/x86/hvm/hvm.c > > > +++ b/xen/arch/x86/hvm/hvm.c > > > @@ -2263,7 +2263,8 @@ int hvm_set_cr0(unsigned long value, bool may_defer) > > > { > > > ASSERT(v->arch.vm_event); > > > > > > - if ( hvm_monitor_crX(CR0, value, old_value) ) > > > + if ( hvm_monitor_crX(CR0, value, old_value) && > > > + v->domain->arch.monitor.control_register_values ) > > > { > > > /* The actual write will occur in hvm_do_resume(), if > > > permitted. */ > > > v->arch.vm_event->write_data.do_write.cr0 = 1; > > > @@ -2362,7 +2363,8 @@ int hvm_set_cr3(unsigned long value, bool may_defer) > > > { > > > ASSERT(v->arch.vm_event); > > > > > > - if ( hvm_monitor_crX(CR3, value, old) ) > > > + if ( hvm_monitor_crX(CR3, value, old) && > > > + v->domain->arch.monitor.control_register_values ) > > > { > > > /* The actual write will occur in hvm_do_resume(), if > > > permitted. */ > > > v->arch.vm_event->write_data.do_write.cr3 = 1; > > > @@ -2443,7 +2445,8 @@ int hvm_set_cr4(unsigned long value, bool may_defer) > > > { > > > ASSERT(v->arch.vm_event); > > > > > > - if ( hvm_monitor_crX(CR4, value, old_cr) ) > > > + if ( hvm_monitor_crX(CR4, value, old_cr) && > > > + v->domain->arch.monitor.control_register_values ) > > > > I think you could return control_register_values in hvm_monitor_crX > > instead of having to add the check to each caller? > > We could, but this way the code is more consistent. OK, I guess it's a matter of taste. I would rather prefer those checks to be confined to hvm_monitor_crX because then the generic code is not polluted with monitor checks, but that's likely just my taste. > > > > > { > > > /* The actual write will occur in hvm_do_resume(), if > > > permitted. */ > > > v->arch.vm_event->write_data.do_write.cr4 = 1; > > > @@ -3587,13 +3590,17 @@ int hvm_msr_write_intercept(unsigned int msr, > > > uint64_t msr_content, > > > > > > ASSERT(v->arch.vm_event); > > > > > > - /* The actual write will occur in hvm_do_resume() (if > > > permitted). */ > > > - v->arch.vm_event->write_data.do_write.msr = 1; > > > - v->arch.vm_event->write_data.msr = msr; > > > - v->arch.vm_event->write_data.value = msr_content; > > > - > > > hvm_monitor_msr(msr, msr_content, msr_old_content); > > > - return X86EMUL_OKAY; > > > + > > > + if ( v->domain->arch.monitor.control_register_values ) > > > > Is there any value in limiting control_register_values to MSR that > > represent control registers, like EFER and XSS? > > I don't know, you would have to ask Bitdefender about it who made this > feature. > > > > > > + { > > > + /* The actual write will occur in hvm_do_resume(), if > > > permitted. */ > > > + v->arch.vm_event->write_data.do_write.msr = 1; > > > + v->arch.vm_event->write_data.msr = msr; > > > + v->arch.vm_event->write_data.value = msr_content; > > > + > > > + return X86EMUL_OKAY; > > > + } > > > > You seem to change the previous flow of the function here, that would > > just call hvm_monitor_msr and return previously. > > > > Don't you need to move the return from outside the added if condition > > in order to keep previous behavior? Or else the write is committed > > straight away. > > That's exactly what we want to achieve. Postponing the write is buggy. > We want to make that feature optional. Before Bitdefender contributed > that feature writes were always commited straight away, so with this > patch we are actually reverting default behavior to what it was like > to start with. Oh, could this be made clear on the commit message then? When I first saw the code I assumed this was wrong (I'm likely not familiar enough with the code anyway). Thanks, Roger.