Re: [PATCH v2 for-4.14 1/3] xen/monitor: Control register values

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.pau@xxxxxxxxxx> 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 <tamas@xxxxxxxxxxxxx>
> > > ---
> > >  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.