Extend the monitor_op domctl to include option that enables
controlling what values certain registers are permitted to hold
by a monitor subscriber.

Signed-off-by: Tamas K Lengyel <tamas@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 )
         {
             /* 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 )
+        {
+            /* 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;
+        }
     }
 
     if ( (ret = guest_wrmsr(v, msr, msr_content)) != X86EMUL_UNHANDLEABLE )
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index bbcb7536c7..1517a97f50 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -144,7 +144,15 @@ int arch_monitor_domctl_event(struct domain *d,
                               struct xen_domctl_monitor_op *mop)
 {
     struct arch_domain *ad = &d->arch;
-    bool requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
+    bool requested_status;
+
+    if ( XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS == mop->op )
+    {
+        ad->monitor.control_register_values = true;
+        return 0;
+    }
+
+    requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
 
     switch ( mop->event )
     {
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 5b6d909266..d890ab7a22 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -416,6 +416,7 @@ struct arch_domain
          * This is used to filter out pagefaults.
          */
         unsigned int inguest_pagefault_disabled                            : 1;
+        unsigned int control_register_values                               : 1;
         struct monitor_msr_bitmap *msr_bitmap;
         uint64_t write_ctrlreg_mask[4];
     } monitor;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 1ad34c35eb..cbcd25f12c 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1025,6 +1025,7 @@ struct xen_domctl_psr_cmt_op {
 #define XEN_DOMCTL_MONITOR_OP_DISABLE           1
 #define XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES  2
 #define XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP  3
+#define XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS 4
 
 #define XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG         0
 #define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR            1
-- 
2.26.1

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?

There already seems to be some support for gating MSR writes, which
seems to be expanded by this commit?

Is it solving some kind of bug reported?

> Signed-off-by: Tamas K Lengyel <tamas@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?

>          {
>              /* 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?

> +        {
> +            /* 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.

>      }
>  
>      if ( (ret = guest_wrmsr(v, msr, msr_content)) != X86EMUL_UNHANDLEABLE )
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index bbcb7536c7..1517a97f50 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -144,7 +144,15 @@ int arch_monitor_domctl_event(struct domain *d,
>                                struct xen_domctl_monitor_op *mop)
>  {
>      struct arch_domain *ad = &d->arch;
> -    bool requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
> +    bool requested_status;
> +
> +    if ( XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS == mop->op )
> +    {
> +        ad->monitor.control_register_values = true;

I think strictly speaking you need to use 1 here, since this variable
is not a boolean.

Thanks, Roger.

On Tue, Jun 2, 2020 at 5:08 AM Roger Pau Monné <roger.pau@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.

>
> > Signed-off-by: Tamas K Lengyel <tamas@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.

>
> >          {
> >              /* 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.

>
> >      }
> >
> >      if ( (ret = guest_wrmsr(v, msr, msr_content)) != X86EMUL_UNHANDLEABLE )
> > diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> > index bbcb7536c7..1517a97f50 100644
> > --- a/xen/arch/x86/monitor.c
> > +++ b/xen/arch/x86/monitor.c
> > @@ -144,7 +144,15 @@ int arch_monitor_domctl_event(struct domain *d,
> >                                struct xen_domctl_monitor_op *mop)
> >  {
> >      struct arch_domain *ad = &d->arch;
> > -    bool requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
> > +    bool requested_status;
> > +
> > +    if ( XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS == mop->op )
> > +    {
> > +        ad->monitor.control_register_values = true;
>
> I think strictly speaking you need to use 1 here, since this variable
> is not a boolean.

Sure.

Thanks,
Tamas

On 02.06.2020 14:40, Tamas K Lengyel wrote:
> On Tue, Jun 2, 2020 at 5:08 AM Roger Pau Monné <roger.pau@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.

Now that's exactly the opposite of what I had derived from the
description here so far. Perhaps an at least weak indication
that you want to reword this. For example, from your reply to
Roger I understand it's rather that the new flag allows to
"suppress" the controlling (since presumably you don't change
default behavior), rather then "enabling" it.

>> 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 can such important aspects also go in the commit message,
so they're available later without needing to hunt down mail
threads (besides this way being more readily available to
reviewers)?

Jan

On Tue, Jun 2, 2020 at 6:47 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.06.2020 14:40, Tamas K Lengyel wrote:
> > On Tue, Jun 2, 2020 at 5:08 AM Roger Pau Monné <roger.pau@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.
>
> Now that's exactly the opposite of what I had derived from the
> description here so far. Perhaps an at least weak indication
> that you want to reword this. For example, from your reply to
> Roger I understand it's rather that the new flag allows to
> "suppress" the controlling (since presumably you don't change
> default behavior), rather then "enabling" it.

What we are adding is a domctl you need to call that enables this
feature. It's not an option to suppress it. It shouldn't have been
enabled by default to begin with. That was a mistake when the feature
was contributed and it is buggy.

>
> >> 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 can such important aspects also go in the commit message,
> so they're available later without needing to hunt down mail
> threads (besides this way being more readily available to
> reviewers)?

Noted.

Thanks,
Tamas

On 02.06.2020 14:51, Tamas K Lengyel wrote:
> On Tue, Jun 2, 2020 at 6:47 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 02.06.2020 14:40, Tamas K Lengyel wrote:
>>> On Tue, Jun 2, 2020 at 5:08 AM Roger Pau Monné <roger.pau@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.
>>
>> Now that's exactly the opposite of what I had derived from the
>> description here so far. Perhaps an at least weak indication
>> that you want to reword this. For example, from your reply to
>> Roger I understand it's rather that the new flag allows to
>> "suppress" the controlling (since presumably you don't change
>> default behavior), rather then "enabling" it.
> 
> What we are adding is a domctl you need to call that enables this
> feature. It's not an option to suppress it. It shouldn't have been
> enabled by default to begin with. That was a mistake when the feature
> was contributed and it is buggy.

Okay, in this case it's important to point out that you alter
default behavior. The BitDefender folks may not like this, yet
they've been surprisingly silent so far.

Jan

On Tue, Jun 2, 2020 at 7:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.06.2020 14:51, Tamas K Lengyel wrote:
> > On Tue, Jun 2, 2020 at 6:47 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 02.06.2020 14:40, Tamas K Lengyel wrote:
> >>> On Tue, Jun 2, 2020 at 5:08 AM Roger Pau Monné <roger.pau@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.
> >>
> >> Now that's exactly the opposite of what I had derived from the
> >> description here so far. Perhaps an at least weak indication
> >> that you want to reword this. For example, from your reply to
> >> Roger I understand it's rather that the new flag allows to
> >> "suppress" the controlling (since presumably you don't change
> >> default behavior), rather then "enabling" it.
> >
> > What we are adding is a domctl you need to call that enables this
> > feature. It's not an option to suppress it. It shouldn't have been
> > enabled by default to begin with. That was a mistake when the feature
> > was contributed and it is buggy.
>
> Okay, in this case it's important to point out that you alter
> default behavior. The BitDefender folks may not like this, yet
> they've been surprisingly silent so far.

Well, it was Bitdefender who altered the default behavior. We are
reverting their mistake and making it optional. But I can certainly
make that more clear.

Tamas

On Tue, Jun 02, 2020 at 07:10:07AM -0600, Tamas K Lengyel wrote:
> On Tue, Jun 2, 2020 at 7:00 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 02.06.2020 14:51, Tamas K Lengyel wrote:
> > > On Tue, Jun 2, 2020 at 6:47 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>
> > >> On 02.06.2020 14:40, Tamas K Lengyel wrote:
> > >>> On Tue, Jun 2, 2020 at 5:08 AM Roger Pau Monné <roger.pau@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.
> > >>
> > >> Now that's exactly the opposite of what I had derived from the
> > >> description here so far. Perhaps an at least weak indication
> > >> that you want to reword this. For example, from your reply to
> > >> Roger I understand it's rather that the new flag allows to
> > >> "suppress" the controlling (since presumably you don't change
> > >> default behavior), rather then "enabling" it.
> > >
> > > What we are adding is a domctl you need to call that enables this
> > > feature. It's not an option to suppress it. It shouldn't have been
> > > enabled by default to begin with. That was a mistake when the feature
> > > was contributed and it is buggy.
> >
> > Okay, in this case it's important to point out that you alter
> > default behavior. The BitDefender folks may not like this, yet
> > they've been surprisingly silent so far.
> 
> Well, it was Bitdefender who altered the default behavior. We are
> reverting their mistake and making it optional. But I can certainly
> make that more clear.

I would like some input from the Bitdefender guys. Is there a bugfix
planned for this feature?

It would be nice to get this in before 4.14 releases.

Roger.

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@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 <tamas@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.

On 02.06.2020 15:01, Roger Pau Monné wrote:
> 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@citrix.com> wrote:
>>> On Wed, May 20, 2020 at 08:31:52PM -0600, Tamas K Lengyel wrote:
>>>> --- 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.

+1

Jan

On Tue, Jun 2, 2020 at 7:04 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.06.2020 15:01, Roger Pau Monné wrote:
> > 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@citrix.com> wrote:
> >>> On Wed, May 20, 2020 at 08:31:52PM -0600, Tamas K Lengyel wrote:
> >>>> --- 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.
>
> +1


OK.

On Tue, Jun 2, 2020 at 7:01 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> 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@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.

Sure. As things stand I'm dropping patch 2 & 3 anyway so I'll just use
the cover letter as the commit message.

Tamas