Xen Developers, In C function cpu_add(), in xen/arch/x86/smpboot.c, if acpi_id == MAX_MADT_ENTRIES, won''t this write past the end of array x86_acpiid_toapicid[MAX_MADT_ENTRIES]? I am looking at xen-unstable. It looks like the guard is not catching this 1 case? Sincerely, John McDermott ---- What is the formal meaning of the one-line program #include "/dev/tty" J.P. McDermott building 12 Code 5542 mcdermott@itd.nrl.navy.mil Naval Research Laboratory voice: +1 202.404.8301 Washington, DC 20375, US fax: +1 202.404.7942 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 09/06/2011 16:49, "John McDermott (U.S. Navy Employee)" <john.mcdermott@nrl.navy.mil> wrote:> Xen Developers, > > In C function cpu_add(), in xen/arch/x86/smpboot.c, if acpi_id => MAX_MADT_ENTRIES, won''t this write past the end of array > x86_acpiid_toapicid[MAX_MADT_ENTRIES]? I am looking at xen-unstable. It looks > like the guard is not catching this 1 case?Looks like it. Also the check against MAX_APICS, and the pxm value against 256, are all similarly off by one. Thanks for spotting it! -- Keir> Sincerely, > > John McDermott > ---- > What is the formal meaning of the one-line program > #include "/dev/tty" > > J.P. McDermott building 12 > Code 5542 mcdermott@itd.nrl.navy.mil > Naval Research Laboratory voice: +1 202.404.8301 > Washington, DC 20375, US fax: +1 202.404.7942 > > > > > > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dong, Eddie
2011-Jun-10 05:36 UTC
[Xen-devel] Addback capability check for non-initial features
add back missing capability check of MSR_IA32_VMX_PROCBASED_CTLS.
Besides initial configuration, adjust_vmx_controls is responsible for
hardware capibility check as well. This patch add back the check.
Signed-off-by: Eddie Dong <eddie.dong@intel.com>
diff -r 43a06a43e60b xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c Thu Jun 09 16:30:34 2011 +0800
+++ b/xen/arch/x86/hvm/vmx/vmcs.c Fri Jun 10 13:28:49 2011 +0800
@@ -148,6 +148,11 @@ static int vmx_init_vmcs_config(void)
MSR_IA32_VMX_PINBASED_CTLS, &mismatch);
min = (CPU_BASED_HLT_EXITING |
+ CPU_BASED_VIRTUAL_INTR_PENDING |
+#ifdef __x86_64__
+ CPU_BASED_CR8_LOAD_EXITING |
+ CPU_BASED_CR8_STORE_EXITING |
+#endif
CPU_BASED_INVLPG_EXITING |
CPU_BASED_CR3_LOAD_EXITING |
CPU_BASED_CR3_STORE_EXITING |
@@ -164,15 +169,12 @@ static int vmx_init_vmcs_config(void)
_vmx_cpu_based_exec_control = adjust_vmx_controls(
"CPU-Based Exec Control", min, opt,
MSR_IA32_VMX_PROCBASED_CTLS, &mismatch);
- _vmx_cpu_based_exec_control &= ~CPU_BASED_RDTSC_EXITING;
+ _vmx_cpu_based_exec_control &= ~(CPU_BASED_RDTSC_EXITING |
+ CPU_BASED_VIRTUAL_INTR_PENDING);
#ifdef __x86_64__
if ( !(_vmx_cpu_based_exec_control & CPU_BASED_TPR_SHADOW) )
- {
- min |= CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING;
- _vmx_cpu_based_exec_control = adjust_vmx_controls(
- "CPU-Based Exec Control", min, opt,
- MSR_IA32_VMX_PROCBASED_CTLS, &mismatch);
- }
+ _vmx_cpu_based_exec_control &= ~(CPU_BASED_CR8_LOAD_EXITING |
+ CPU_BASED_CR8_STORE_EXITING);
#endif
if ( _vmx_cpu_based_exec_control &
CPU_BASED_ACTIVATE_SECONDARY_CONTROLS )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Dong, Eddie
2011-Jun-10 05:50 UTC
[Xen-devel] Addback capability check for non-initial features
add back missing capability check of MSR_IA32_VMX_PROCBASED_CTLS.
Besides initial configuration, adjust_vmx_controls is responsible for
hardware capibility check as well. This patch add back the check.
Signed-off-by: Eddie Dong <eddie.dong@intel.com>
diff -r 43a06a43e60b xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c Thu Jun 09 16:30:34 2011 +0800
+++ b/xen/arch/x86/hvm/vmx/vmcs.c Fri Jun 10 13:28:49 2011 +0800
@@ -148,6 +148,11 @@ static int vmx_init_vmcs_config(void)
MSR_IA32_VMX_PINBASED_CTLS, &mismatch);
min = (CPU_BASED_HLT_EXITING |
+ CPU_BASED_VIRTUAL_INTR_PENDING |
+#ifdef __x86_64__
+ CPU_BASED_CR8_LOAD_EXITING |
+ CPU_BASED_CR8_STORE_EXITING |
+#endif
CPU_BASED_INVLPG_EXITING |
CPU_BASED_CR3_LOAD_EXITING |
CPU_BASED_CR3_STORE_EXITING |
@@ -164,15 +169,12 @@ static int vmx_init_vmcs_config(void)
_vmx_cpu_based_exec_control = adjust_vmx_controls(
"CPU-Based Exec Control", min, opt,
MSR_IA32_VMX_PROCBASED_CTLS, &mismatch);
- _vmx_cpu_based_exec_control &= ~CPU_BASED_RDTSC_EXITING;
+ _vmx_cpu_based_exec_control &= ~(CPU_BASED_RDTSC_EXITING |
+ CPU_BASED_VIRTUAL_INTR_PENDING);
#ifdef __x86_64__
if ( !(_vmx_cpu_based_exec_control & CPU_BASED_TPR_SHADOW) )
- {
- min |= CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING;
- _vmx_cpu_based_exec_control = adjust_vmx_controls(
- "CPU-Based Exec Control", min, opt,
- MSR_IA32_VMX_PROCBASED_CTLS, &mismatch);
- }
+ _vmx_cpu_based_exec_control &= ~(CPU_BASED_CR8_LOAD_EXITING |
+ CPU_BASED_CR8_STORE_EXITING);
#endif
if ( _vmx_cpu_based_exec_control &
CPU_BASED_ACTIVATE_SECONDARY_CONTROLS )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2011-Jun-10 06:05 UTC
[Xen-devel] Re: Addback capability check for non-initial features
On 10/06/2011 06:50, "Dong, Eddie" <eddie.dong@intel.com> wrote:> > add back missing capability check of MSR_IA32_VMX_PROCBASED_CTLS. > > Besides initial configuration, adjust_vmx_controls is responsible for > hardware capibility check as well. This patch add back the check.I suppose the CPU_BASED_VIRTUAL_INTR_PENDING addition is correct, for what it''s worth (surely every VMX-capable CPU ever has and will support that). The change to CR8 detection looks mad and incorrect. You''ve inverted it so that CR8 exits get enabled when TPR_SHADOW is available, rather than when it isn''t, surely? And that can''t be correct. I don''t see how the CR8-exit detection and enabling is wrong, as it is already. -- Keir> Signed-off-by: Eddie Dong <eddie.dong@intel.com> > > diff -r 43a06a43e60b xen/arch/x86/hvm/vmx/vmcs.c > --- a/xen/arch/x86/hvm/vmx/vmcs.c Thu Jun 09 16:30:34 2011 +0800 > +++ b/xen/arch/x86/hvm/vmx/vmcs.c Fri Jun 10 13:28:49 2011 +0800 > @@ -148,6 +148,11 @@ static int vmx_init_vmcs_config(void) > MSR_IA32_VMX_PINBASED_CTLS, &mismatch); > > min = (CPU_BASED_HLT_EXITING | > + CPU_BASED_VIRTUAL_INTR_PENDING | > +#ifdef __x86_64__ > + CPU_BASED_CR8_LOAD_EXITING | > + CPU_BASED_CR8_STORE_EXITING | > +#endif > CPU_BASED_INVLPG_EXITING | > CPU_BASED_CR3_LOAD_EXITING | > CPU_BASED_CR3_STORE_EXITING | > @@ -164,15 +169,12 @@ static int vmx_init_vmcs_config(void) > _vmx_cpu_based_exec_control = adjust_vmx_controls( > "CPU-Based Exec Control", min, opt, > MSR_IA32_VMX_PROCBASED_CTLS, &mismatch); > - _vmx_cpu_based_exec_control &= ~CPU_BASED_RDTSC_EXITING; > + _vmx_cpu_based_exec_control &= ~(CPU_BASED_RDTSC_EXITING | > + CPU_BASED_VIRTUAL_INTR_PENDING); > #ifdef __x86_64__ > if ( !(_vmx_cpu_based_exec_control & CPU_BASED_TPR_SHADOW) ) > - { > - min |= CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING; > - _vmx_cpu_based_exec_control = adjust_vmx_controls( > - "CPU-Based Exec Control", min, opt, > - MSR_IA32_VMX_PROCBASED_CTLS, &mismatch); > - } > + _vmx_cpu_based_exec_control &= ~(CPU_BASED_CR8_LOAD_EXITING | > + CPU_BASED_CR8_STORE_EXITING); > #endif > > if ( _vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS > )_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dong, Eddie
2011-Jun-10 06:33 UTC
[Xen-devel] RE: Addback capability check for non-initial features
> > > > add back missing capability check of MSR_IA32_VMX_PROCBASED_CTLS. > > > > Besides initial configuration, adjust_vmx_controls is responsible for > > hardware capibility check as well. This patch add back the check. > > I suppose the CPU_BASED_VIRTUAL_INTR_PENDING addition is correct, for > what > it''s worth (surely every VMX-capable CPU ever has and will support that). > > The change to CR8 detection looks mad and incorrect. You''ve inverted it so > that CR8 exits get enabled when TPR_SHADOW is available, rather than > when itCR8 exit is removed later on if TPR_SHADOW exist:) The only difference is that if there are processors that support TPR_SHADOW only, I can check internally if this is the concern. Current nested vmx is assuming CR8 exiting is presented to emulate L1 guest CR8 exiting. TPR_SHAOW can''t trap CR8 read though cr8 write trap is OK w/ TPR shadow. Eventually I want to have a minimal common set of capability that is supported by all HW and is presented to L1 guest.> isn''t, surely? And that can''t be correct. I don''t see how the CR8-exit > detection and enabling is wrong, as it is already.The original code for CR8 exit is correct too :) Thx, Eddie _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Jun-10 06:58 UTC
[Xen-devel] Re: Addback capability check for non-initial features
On 10/06/2011 07:33, "Dong, Eddie" <eddie.dong@intel.com> wrote:>>> >>> add back missing capability check of MSR_IA32_VMX_PROCBASED_CTLS. >>> >>> Besides initial configuration, adjust_vmx_controls is responsible for >>> hardware capibility check as well. This patch add back the check. >> >> I suppose the CPU_BASED_VIRTUAL_INTR_PENDING addition is correct, for >> what >> it''s worth (surely every VMX-capable CPU ever has and will support that). >> >> The change to CR8 detection looks mad and incorrect. You''ve inverted it so >> that CR8 exits get enabled when TPR_SHADOW is available, rather than >> when it > > CR8 exit is removed later on if TPR_SHADOW exist:)Not in your patch. You remove it later if TPR_SHADOW *doesn''t* exist.> The only difference is that if there are processors that support TPR_SHADOW > only, I can check internally if this is the concern. > Current nested vmx is assuming CR8 exiting is presented to emulate L1 guest > CR8 exiting. TPR_SHAOW can''t trap CR8 read though cr8 write trap is OK w/ TPR > shadow.Hmm okay.> Eventually I want to have a minimal common set of capability that is supported > by all HW and is presented to L1 guest. > >> isn''t, surely? And that can''t be correct. I don''t see how the CR8-exit >> detection and enabling is wrong, as it is already. > > The original code for CR8 exit is correct too :)More correct than yours :) -- Keir> Thx, Eddie_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 09/06/2011 16:49, "John McDermott (U.S. Navy Employee)" <john.mcdermott@nrl.navy.mil> wrote:> Xen Developers, > > In C function cpu_add(), in xen/arch/x86/smpboot.c, if acpi_id => MAX_MADT_ENTRIES, won''t this write past the end of array > x86_acpiid_toapicid[MAX_MADT_ENTRIES]? I am looking at xen-unstable. It looks > like the guard is not catching this 1 case?Fixed in xen-unstable:23505. Fortunately this function is only accessible from the TCB so it''s not exploitable. Thanks, -- Keir> Sincerely, > > John McDermott > ---- > What is the formal meaning of the one-line program > #include "/dev/tty" > > J.P. McDermott building 12 > Code 5542 mcdermott@itd.nrl.navy.mil > Naval Research Laboratory voice: +1 202.404.8301 > Washington, DC 20375, US fax: +1 202.404.7942 > > > > > > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Jun-10 07:33 UTC
[Xen-devel] Re: Addback capability check for non-initial features
On 10/06/2011 06:50, "Dong, Eddie" <eddie.dong@intel.com> wrote:> > add back missing capability check of MSR_IA32_VMX_PROCBASED_CTLS. > > Besides initial configuration, adjust_vmx_controls is responsible for > hardware capibility check as well. This patch add back the check.I''ve fixed this and then applied it as xen-unstable:23508. Please take a look. -- Keir> Signed-off-by: Eddie Dong <eddie.dong@intel.com> > > diff -r 43a06a43e60b xen/arch/x86/hvm/vmx/vmcs.c > --- a/xen/arch/x86/hvm/vmx/vmcs.c Thu Jun 09 16:30:34 2011 +0800 > +++ b/xen/arch/x86/hvm/vmx/vmcs.c Fri Jun 10 13:28:49 2011 +0800 > @@ -148,6 +148,11 @@ static int vmx_init_vmcs_config(void) > MSR_IA32_VMX_PINBASED_CTLS, &mismatch); > > min = (CPU_BASED_HLT_EXITING | > + CPU_BASED_VIRTUAL_INTR_PENDING | > +#ifdef __x86_64__ > + CPU_BASED_CR8_LOAD_EXITING | > + CPU_BASED_CR8_STORE_EXITING | > +#endif > CPU_BASED_INVLPG_EXITING | > CPU_BASED_CR3_LOAD_EXITING | > CPU_BASED_CR3_STORE_EXITING | > @@ -164,15 +169,12 @@ static int vmx_init_vmcs_config(void) > _vmx_cpu_based_exec_control = adjust_vmx_controls( > "CPU-Based Exec Control", min, opt, > MSR_IA32_VMX_PROCBASED_CTLS, &mismatch); > - _vmx_cpu_based_exec_control &= ~CPU_BASED_RDTSC_EXITING; > + _vmx_cpu_based_exec_control &= ~(CPU_BASED_RDTSC_EXITING | > + CPU_BASED_VIRTUAL_INTR_PENDING); > #ifdef __x86_64__ > if ( !(_vmx_cpu_based_exec_control & CPU_BASED_TPR_SHADOW) ) > - { > - min |= CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING; > - _vmx_cpu_based_exec_control = adjust_vmx_controls( > - "CPU-Based Exec Control", min, opt, > - MSR_IA32_VMX_PROCBASED_CTLS, &mismatch); > - } > + _vmx_cpu_based_exec_control &= ~(CPU_BASED_CR8_LOAD_EXITING | > + CPU_BASED_CR8_STORE_EXITING); > #endif > > if ( _vmx_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS > )_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dong, Eddie
2011-Jun-10 08:05 UTC
[Xen-devel] RE: Addback capability check for non-initial features
Thanks! You are correct. Eddie> -----Original Message----- > From: Keir Fraser [mailto:keir.xen@gmail.com] On Behalf Of Keir Fraser > Sent: Friday, June 10, 2011 3:34 PM > To: Dong, Eddie > Cc: xen-devel@lists.xensource.com > Subject: Re: Addback capability check for non-initial features > > On 10/06/2011 06:50, "Dong, Eddie" <eddie.dong@intel.com> wrote: > > > > > add back missing capability check of MSR_IA32_VMX_PROCBASED_CTLS. > > > > Besides initial configuration, adjust_vmx_controls is responsible for > > hardware capibility check as well. This patch add back the check. > > I''ve fixed this and then applied it as xen-unstable:23508. Please take a > look. > > -- Keir > > > Signed-off-by: Eddie Dong <eddie.dong@intel.com> > > > > diff -r 43a06a43e60b xen/arch/x86/hvm/vmx/vmcs.c > > --- a/xen/arch/x86/hvm/vmx/vmcs.c Thu Jun 09 16:30:34 2011 +0800 > > +++ b/xen/arch/x86/hvm/vmx/vmcs.c Fri Jun 10 13:28:49 2011 +0800 > > @@ -148,6 +148,11 @@ static int vmx_init_vmcs_config(void) > > MSR_IA32_VMX_PINBASED_CTLS, &mismatch); > > > > min = (CPU_BASED_HLT_EXITING | > > + CPU_BASED_VIRTUAL_INTR_PENDING | > > +#ifdef __x86_64__ > > + CPU_BASED_CR8_LOAD_EXITING | > > + CPU_BASED_CR8_STORE_EXITING | > > +#endif > > CPU_BASED_INVLPG_EXITING | > > CPU_BASED_CR3_LOAD_EXITING | > > CPU_BASED_CR3_STORE_EXITING | > > @@ -164,15 +169,12 @@ static int vmx_init_vmcs_config(void) > > _vmx_cpu_based_exec_control = adjust_vmx_controls( > > "CPU-Based Exec Control", min, opt, > > MSR_IA32_VMX_PROCBASED_CTLS, &mismatch); > > - _vmx_cpu_based_exec_control &= ~CPU_BASED_RDTSC_EXITING; > > + _vmx_cpu_based_exec_control &= ~(CPU_BASED_RDTSC_EXITING | > > + > CPU_BASED_VIRTUAL_INTR_PENDING); > > #ifdef __x86_64__ > > if ( !(_vmx_cpu_based_exec_control & > CPU_BASED_TPR_SHADOW) ) > > - { > > - min |= CPU_BASED_CR8_LOAD_EXITING | > CPU_BASED_CR8_STORE_EXITING; > > - _vmx_cpu_based_exec_control = adjust_vmx_controls( > > - "CPU-Based Exec Control", min, opt, > > - MSR_IA32_VMX_PROCBASED_CTLS, &mismatch); > > - } > > + _vmx_cpu_based_exec_control &> ~(CPU_BASED_CR8_LOAD_EXITING | > > + > CPU_BASED_CR8_STORE_EXITING); > > #endif > > > > if ( _vmx_cpu_based_exec_control & > CPU_BASED_ACTIVATE_SECONDARY_CONTROLS > > ) >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir, Thanks; yes, we see no way to unravel it. I''m just paranoid. Sincerely, John On Jun 10, 2011, at 3:30 AM, Keir Fraser wrote:> On 09/06/2011 16:49, "John McDermott (U.S. Navy Employee)" > <john.mcdermott@nrl.navy.mil> wrote: > >> Xen Developers, >> >> In C function cpu_add(), in xen/arch/x86/smpboot.c, if acpi_id =>> MAX_MADT_ENTRIES, won''t this write past the end of array >> x86_acpiid_toapicid[MAX_MADT_ENTRIES]? I am looking at xen-unstable. It looks >> like the guard is not catching this 1 case? > > Fixed in xen-unstable:23505. Fortunately this function is only accessible > from the TCB so it''s not exploitable. > > Thanks, > -- Keir > >> Sincerely, >> >> John McDermott >> ---- >> What is the formal meaning of the one-line program >> #include "/dev/tty" >> >> J.P. McDermott building 12 >> Code 5542 mcdermott@itd.nrl.navy.mil >> Naval Research Laboratory voice: +1 202.404.8301 >> Washington, DC 20375, US fax: +1 202.404.7942 >> >> >> >> >> >> >> >> >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel >---- What is the formal meaning of the one-line program #include "/dev/tty" J.P. McDermott building 12 Code 5542 mcdermott@itd.nrl.navy.mil Naval Research Laboratory voice: +1 202.404.8301 Washington, DC 20375, US fax: +1 202.404.7942 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel