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.
