diff mbox series

[v3,7/8] x86/hvm: Disallow access to unknown MSRs

Message ID 20200901105445.22277-8-roger.pau@citrix.com
State Superseded
Headers show
Series x86: switch default MSR behavior | expand

Commit Message

Roger Pau Monne Sept. 1, 2020, 10:54 a.m. UTC
From: Andrew Cooper <andrew.cooper3@citrix.com>

Change the catch-all behavior for MSR not explicitly handled. Instead
of allow full read-access to the MSR space and silently dropping
writes return an exception when the MSR is not explicitly handled.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
[remove rdmsr_safe from default case in svm_msr_read_intercept]
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Fold chunk to remove explicit write handling of VMX MSRs just to
   #GP.
 - Remove catch-all rdmsr_safe in svm_msr_read_intercept.
---
 xen/arch/x86/hvm/svm/svm.c | 10 ++++------
 xen/arch/x86/hvm/vmx/vmx.c | 16 ++++------------
 2 files changed, 8 insertions(+), 18 deletions(-)

Comments

Jan Beulich Sept. 4, 2020, 8:53 a.m. UTC | #1
On 01.09.2020 12:54, Roger Pau Monne wrote:
> @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>          __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
>          break;
>  
> -    case MSR_IA32_FEATURE_CONTROL:
> -    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
> -        /* None of these MSRs are writeable. */
> -        goto gp_fault;

I understand Andrew did ask for this (and I didn't look closely
when I saw the comment), but ...

> @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>               is_last_branch_msr(msr) )
>              break;
>  
> -        /* Match up with the RDMSR side; ultimately this should go away. */
> -        if ( rdmsr_safe(msr, msr_content) == 0 )
> -            break;
> -
> +        gdprintk(XENLOG_WARNING,
> +                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> +                 msr, msr_content);
>          goto gp_fault;

... above from here is logic that handling of these MSRs now goes
through. I'm particularly worried about vmx_write_guest_msr(),
which blindly updates the value of any MSR it can find, i.e. if
any r/o MSR (from the set above, or even more generally) ever got
added to this vCPU-specific set, the r/o-ness would no longer be
maintained. Do we perhaps need to white-list MSRs for which
vmx_write_guest_msr() may get called here?

Jan
Andrew Cooper Sept. 4, 2020, 9:44 a.m. UTC | #2
On 04/09/2020 09:53, Jan Beulich wrote:
> On 01.09.2020 12:54, Roger Pau Monne wrote:
>> @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>          __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
>>          break;
>>  
>> -    case MSR_IA32_FEATURE_CONTROL:
>> -    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
>> -        /* None of these MSRs are writeable. */
>> -        goto gp_fault;
> I understand Andrew did ask for this (and I didn't look closely
> when I saw the comment), but ...
>
>> @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>               is_last_branch_msr(msr) )
>>              break;
>>  
>> -        /* Match up with the RDMSR side; ultimately this should go away. */
>> -        if ( rdmsr_safe(msr, msr_content) == 0 )
>> -            break;
>> -
>> +        gdprintk(XENLOG_WARNING,
>> +                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
>> +                 msr, msr_content);
>>          goto gp_fault;
> ... above from here is logic that handling of these MSRs now goes
> through. I'm particularly worried about vmx_write_guest_msr(),
> which blindly updates the value of any MSR it can find, i.e. if
> any r/o MSR (from the set above, or even more generally) ever got
> added to this vCPU-specific set, the r/o-ness would no longer be
> maintained. Do we perhaps need to white-list MSRs for which
> vmx_write_guest_msr() may get called here?

If a read-only MSR ever actually gets into the load/save list, we'll
take a VMEntry failure (guest load) or SHUTDOWN (host load) as a
consequence of microcode takes a #GP fault.

However, restricting the content of this catch-all clause to nothing
(but the printk) is something I have planned as part of the ongoing MSR
cleanup work.

~Andrew
Jan Beulich Sept. 4, 2020, 9:58 a.m. UTC | #3
On 04.09.2020 11:44, Andrew Cooper wrote:
> On 04/09/2020 09:53, Jan Beulich wrote:
>> On 01.09.2020 12:54, Roger Pau Monne wrote:
>>> @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>>          __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
>>>          break;
>>>  
>>> -    case MSR_IA32_FEATURE_CONTROL:
>>> -    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
>>> -        /* None of these MSRs are writeable. */
>>> -        goto gp_fault;
>> I understand Andrew did ask for this (and I didn't look closely
>> when I saw the comment), but ...
>>
>>> @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>>               is_last_branch_msr(msr) )
>>>              break;
>>>  
>>> -        /* Match up with the RDMSR side; ultimately this should go away. */
>>> -        if ( rdmsr_safe(msr, msr_content) == 0 )
>>> -            break;
>>> -
>>> +        gdprintk(XENLOG_WARNING,
>>> +                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
>>> +                 msr, msr_content);
>>>          goto gp_fault;
>> ... above from here is logic that handling of these MSRs now goes
>> through. I'm particularly worried about vmx_write_guest_msr(),
>> which blindly updates the value of any MSR it can find, i.e. if
>> any r/o MSR (from the set above, or even more generally) ever got
>> added to this vCPU-specific set, the r/o-ness would no longer be
>> maintained. Do we perhaps need to white-list MSRs for which
>> vmx_write_guest_msr() may get called here?
> 
> If a read-only MSR ever actually gets into the load/save list, we'll
> take a VMEntry failure (guest load) or SHUTDOWN (host load) as a
> consequence of microcode takes a #GP fault.

Oh, of course. In order to surface a value different from the hardware's
one has to intercept such MSRs.

> However, restricting the content of this catch-all clause to nothing
> (but the printk) is something I have planned as part of the ongoing MSR
> cleanup work.

Good to know.

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Roger Pau Monne Sept. 4, 2020, 11:13 a.m. UTC | #4
On Fri, Sep 04, 2020 at 10:53:26AM +0200, Jan Beulich wrote:
> On 01.09.2020 12:54, Roger Pau Monne wrote:
> > @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
> >          __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
> >          break;
> >  
> > -    case MSR_IA32_FEATURE_CONTROL:
> > -    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
> > -        /* None of these MSRs are writeable. */
> > -        goto gp_fault;
> 
> I understand Andrew did ask for this (and I didn't look closely
> when I saw the comment), but ...
> 
> > @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
> >               is_last_branch_msr(msr) )
> >              break;
> >  
> > -        /* Match up with the RDMSR side; ultimately this should go away. */
> > -        if ( rdmsr_safe(msr, msr_content) == 0 )
> > -            break;
> > -
> > +        gdprintk(XENLOG_WARNING,
> > +                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> > +                 msr, msr_content);
> >          goto gp_fault;
> 
> ... above from here is logic that handling of these MSRs now goes
> through. I'm particularly worried about vmx_write_guest_msr(),
> which blindly updates the value of any MSR it can find, i.e. if
> any r/o MSR (from the set above, or even more generally) ever got
> added to this vCPU-specific set, the r/o-ness would no longer be
> maintained.

But those MSRs need to be added to the list explicitly (using
vmx_add_guest_msr) in order for the guest to be able to update them,
and they are supposed to be owned by the guest?

I understand the concern, but AFAICT none of the MSRs handled by
VMX_MSR_GUEST require such handling. Maybe it's worth adding something
like VMX_MSR_GUEST_RO in the future if such need arises?

> Do we perhaps need to white-list MSRs for which
> vmx_write_guest_msr() may get called here?

When such MSRs are added such addition should make sure they are not
allowed write permissions? You would have to do that now anyway
AFAICT.

Roger.
Tian, Kevin Sept. 7, 2020, 3:31 a.m. UTC | #5
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Tuesday, September 1, 2020 6:55 PM
> 
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Change the catch-all behavior for MSR not explicitly handled. Instead
> of allow full read-access to the MSR space and silently dropping
> writes return an exception when the MSR is not explicitly handled.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> [remove rdmsr_safe from default case in svm_msr_read_intercept]
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
> Changes since v1:
>  - Fold chunk to remove explicit write handling of VMX MSRs just to
>    #GP.
>  - Remove catch-all rdmsr_safe in svm_msr_read_intercept.
> ---
>  xen/arch/x86/hvm/svm/svm.c | 10 ++++------
>  xen/arch/x86/hvm/vmx/vmx.c | 16 ++++------------
>  2 files changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 0e43154c7e..66b22efdab 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1964,8 +1964,7 @@ static int svm_msr_read_intercept(unsigned int
> msr, uint64_t *msr_content)
>          break;
> 
>      default:
> -        if ( rdmsr_safe(msr, *msr_content) == 0 )
> -            break;
> +        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n",
> msr);
>          goto gpf;
>      }
> 
> @@ -2150,10 +2149,9 @@ static int svm_msr_write_intercept(unsigned int
> msr, uint64_t msr_content)
>          break;
> 
>      default:
> -        /* Match up with the RDMSR side; ultimately this should go away. */
> -        if ( rdmsr_safe(msr, msr_content) == 0 )
> -            break;
> -
> +        gdprintk(XENLOG_WARNING,
> +                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> +                 msr, msr_content);
>          goto gpf;
>      }
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index f6657af923..9cc9d81c41 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3015,9 +3015,7 @@ static int vmx_msr_read_intercept(unsigned int
> msr, uint64_t *msr_content)
>              break;
>          }
> 
> -        if ( rdmsr_safe(msr, *msr_content) == 0 )
> -            break;
> -
> +        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n",
> msr);
>          goto gp_fault;
>      }
> 
> @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int
> msr, uint64_t msr_content)
>          __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
>          break;
> 
> -    case MSR_IA32_FEATURE_CONTROL:
> -    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
> -        /* None of these MSRs are writeable. */
> -        goto gp_fault;
> -
>      case MSR_IA32_MISC_ENABLE:
>          /* Silently drop writes that don't change the reported value. */
>          if ( vmx_msr_read_intercept(msr, &tmp) != X86EMUL_OKAY ||
> @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int
> msr, uint64_t msr_content)
>               is_last_branch_msr(msr) )
>              break;
> 
> -        /* Match up with the RDMSR side; ultimately this should go away. */
> -        if ( rdmsr_safe(msr, msr_content) == 0 )
> -            break;
> -
> +        gdprintk(XENLOG_WARNING,
> +                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> +                 msr, msr_content);
>          goto gp_fault;
>      }
> 
> --
> 2.28.0
diff mbox series

Patch

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 0e43154c7e..66b22efdab 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1964,8 +1964,7 @@  static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         break;
 
     default:
-        if ( rdmsr_safe(msr, *msr_content) == 0 )
-            break;
+        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
         goto gpf;
     }
 
@@ -2150,10 +2149,9 @@  static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         break;
 
     default:
-        /* Match up with the RDMSR side; ultimately this should go away. */
-        if ( rdmsr_safe(msr, msr_content) == 0 )
-            break;
-
+        gdprintk(XENLOG_WARNING,
+                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
+                 msr, msr_content);
         goto gpf;
     }
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f6657af923..9cc9d81c41 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3015,9 +3015,7 @@  static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
             break;
         }
 
-        if ( rdmsr_safe(msr, *msr_content) == 0 )
-            break;
-
+        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
         goto gp_fault;
     }
 
@@ -3290,11 +3288,6 @@  static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
         break;
 
-    case MSR_IA32_FEATURE_CONTROL:
-    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
-        /* None of these MSRs are writeable. */
-        goto gp_fault;
-
     case MSR_IA32_MISC_ENABLE:
         /* Silently drop writes that don't change the reported value. */
         if ( vmx_msr_read_intercept(msr, &tmp) != X86EMUL_OKAY ||
@@ -3320,10 +3313,9 @@  static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
              is_last_branch_msr(msr) )
             break;
 
-        /* Match up with the RDMSR side; ultimately this should go away. */
-        if ( rdmsr_safe(msr, msr_content) == 0 )
-            break;
-
+        gdprintk(XENLOG_WARNING,
+                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
+                 msr, msr_content);
         goto gp_fault;
     }