| Message ID | 1466440225-4161-4-git-send-email-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote: > @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled); > > #ifndef opt_hvm_fep > /* Permit use of the Forced Emulation Prefix in HVM guests */ > -bool_t opt_hvm_fep; > +bool_t __read_mostly opt_hvm_fep; > boolean_param("hvm_fep", opt_hvm_fep); > #endif > +static const char __initconst *warning_hvm_fep = > + "**********************************************\n" > + "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n" > + "******* This option is *ONLY* intended to aid testing of Xen.\n" > + "******* It has implications on the security of the system.\n" > + "******* Please *DO NOT* use this in production.\n" > + "**********************************************\n"; > + > Along with the same comment as on patch 2, here even more than there I wonder whether this string wouldn't better be a static local in hvm_enable() (or even the scope therein where warning_add() gets invoked). Also adding a stray blank line. Jan
On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote: > >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote: > > @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled); > > > > #ifndef opt_hvm_fep > > /* Permit use of the Forced Emulation Prefix in HVM guests */ > > -bool_t opt_hvm_fep; > > +bool_t __read_mostly opt_hvm_fep; > > boolean_param("hvm_fep", opt_hvm_fep); > > #endif > > +static const char __initconst *warning_hvm_fep = > > + "**********************************************\n" > > + "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n" > > + "******* This option is *ONLY* intended to aid testing of Xen.\n" > > + "******* It has implications on the security of the system.\n" > > + "******* Please *DO NOT* use this in production.\n" > > + "**********************************************\n"; > > + > > > > Along with the same comment as on patch 2, here even more than > there I wonder whether this string wouldn't better be a static local > in hvm_enable() (or even the scope therein where warning_add() > gets invoked). > I would rather the text stays next to where the option is defined so it is obvious to anyone who touches this area of code. > Also adding a stray blank line. > Fixed. Also fixed the issue mentioned in patch 2. Wei. > Jan >
>>> On 23.06.16 at 12:50, <wei.liu2@citrix.com> wrote: > On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote: >> >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote: >> > @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled); >> > >> > #ifndef opt_hvm_fep >> > /* Permit use of the Forced Emulation Prefix in HVM guests */ >> > -bool_t opt_hvm_fep; >> > +bool_t __read_mostly opt_hvm_fep; >> > boolean_param("hvm_fep", opt_hvm_fep); >> > #endif >> > +static const char __initconst *warning_hvm_fep = >> > + "**********************************************\n" >> > + "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n" >> > + "******* This option is *ONLY* intended to aid testing of Xen.\n" >> > + "******* It has implications on the security of the system.\n" >> > + "******* Please *DO NOT* use this in production.\n" >> > + "**********************************************\n"; >> > + >> > >> >> Along with the same comment as on patch 2, here even more than >> there I wonder whether this string wouldn't better be a static local >> in hvm_enable() (or even the scope therein where warning_add() >> gets invoked). > > I would rather the text stays next to where the option is defined so > it is obvious to anyone who touches this area of code. Makes sense. But then - shouldn't it move inside the #ifndef? Jan
On Thu, Jun 23, 2016 at 06:20:38AM -0600, Jan Beulich wrote: > >>> On 23.06.16 at 12:50, <wei.liu2@citrix.com> wrote: > > On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote: > >> >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote: > >> > @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled); > >> > > >> > #ifndef opt_hvm_fep > >> > /* Permit use of the Forced Emulation Prefix in HVM guests */ > >> > -bool_t opt_hvm_fep; > >> > +bool_t __read_mostly opt_hvm_fep; > >> > boolean_param("hvm_fep", opt_hvm_fep); > >> > #endif > >> > +static const char __initconst *warning_hvm_fep = > >> > + "**********************************************\n" > >> > + "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n" > >> > + "******* This option is *ONLY* intended to aid testing of Xen.\n" > >> > + "******* It has implications on the security of the system.\n" > >> > + "******* Please *DO NOT* use this in production.\n" > >> > + "**********************************************\n"; > >> > + > >> > > >> > >> Along with the same comment as on patch 2, here even more than > >> there I wonder whether this string wouldn't better be a static local > >> in hvm_enable() (or even the scope therein where warning_add() > >> gets invoked). > > > > I would rather the text stays next to where the option is defined so > > it is obvious to anyone who touches this area of code. > > Makes sense. But then - shouldn't it move inside the #ifndef? > That would then require the warning_add() call to be wrapped in ifdef, too. That looks a bit cumbersome to me. Wei. > Jan >
On 23/06/16 13:44, Wei Liu wrote: > On Thu, Jun 23, 2016 at 06:20:38AM -0600, Jan Beulich wrote: >>>>> On 23.06.16 at 12:50, <wei.liu2@citrix.com> wrote: >>> On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote: >>>>>>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote: >>>>> @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled); >>>>> >>>>> #ifndef opt_hvm_fep >>>>> /* Permit use of the Forced Emulation Prefix in HVM guests */ >>>>> -bool_t opt_hvm_fep; >>>>> +bool_t __read_mostly opt_hvm_fep; >>>>> boolean_param("hvm_fep", opt_hvm_fep); >>>>> #endif >>>>> +static const char __initconst *warning_hvm_fep = >>>>> + "**********************************************\n" >>>>> + "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n" >>>>> + "******* This option is *ONLY* intended to aid testing of Xen.\n" >>>>> + "******* It has implications on the security of the system.\n" >>>>> + "******* Please *DO NOT* use this in production.\n" >>>>> + "**********************************************\n"; >>>>> + >>>>> >>>> Along with the same comment as on patch 2, here even more than >>>> there I wonder whether this string wouldn't better be a static local >>>> in hvm_enable() (or even the scope therein where warning_add() >>>> gets invoked). >>> I would rather the text stays next to where the option is defined so >>> it is obvious to anyone who touches this area of code. >> Makes sense. But then - shouldn't it move inside the #ifndef? >> > That would then require the warning_add() call to be wrapped in ifdef, > too. That looks a bit cumbersome to me. You can do something like: #ifndef $FOO #define warning_text NULL #else const static __initdata warning_text[] = "BAR"; #endif This works in the same way as the current opt_hvm_fep define. ~Andrew
On Thu, Jun 23, 2016 at 01:48:35PM +0100, Andrew Cooper wrote: > On 23/06/16 13:44, Wei Liu wrote: > > On Thu, Jun 23, 2016 at 06:20:38AM -0600, Jan Beulich wrote: > >>>>> On 23.06.16 at 12:50, <wei.liu2@citrix.com> wrote: > >>> On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote: > >>>>>>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote: > >>>>> @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled); > >>>>> > >>>>> #ifndef opt_hvm_fep > >>>>> /* Permit use of the Forced Emulation Prefix in HVM guests */ > >>>>> -bool_t opt_hvm_fep; > >>>>> +bool_t __read_mostly opt_hvm_fep; > >>>>> boolean_param("hvm_fep", opt_hvm_fep); > >>>>> #endif > >>>>> +static const char __initconst *warning_hvm_fep = > >>>>> + "**********************************************\n" > >>>>> + "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n" > >>>>> + "******* This option is *ONLY* intended to aid testing of Xen.\n" > >>>>> + "******* It has implications on the security of the system.\n" > >>>>> + "******* Please *DO NOT* use this in production.\n" > >>>>> + "**********************************************\n"; > >>>>> + > >>>>> > >>>> Along with the same comment as on patch 2, here even more than > >>>> there I wonder whether this string wouldn't better be a static local > >>>> in hvm_enable() (or even the scope therein where warning_add() > >>>> gets invoked). > >>> I would rather the text stays next to where the option is defined so > >>> it is obvious to anyone who touches this area of code. > >> Makes sense. But then - shouldn't it move inside the #ifndef? > >> > > That would then require the warning_add() call to be wrapped in ifdef, > > too. That looks a bit cumbersome to me. > > You can do something like: > > #ifndef $FOO > #define warning_text NULL > #else > const static __initdata warning_text[] = "BAR"; > #endif > > This works in the same way as the current opt_hvm_fep define. > Sure, that works for me, too. Wei. > ~Andrew
>>> On 23.06.16 at 14:44, <wei.liu2@citrix.com> wrote: > On Thu, Jun 23, 2016 at 06:20:38AM -0600, Jan Beulich wrote: >> >>> On 23.06.16 at 12:50, <wei.liu2@citrix.com> wrote: >> > On Wed, Jun 22, 2016 at 09:42:38AM -0600, Jan Beulich wrote: >> >> >>> On 20.06.16 at 18:30, <wei.liu2@citrix.com> wrote: >> >> > @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled); >> >> > >> >> > #ifndef opt_hvm_fep >> >> > /* Permit use of the Forced Emulation Prefix in HVM guests */ >> >> > -bool_t opt_hvm_fep; >> >> > +bool_t __read_mostly opt_hvm_fep; >> >> > boolean_param("hvm_fep", opt_hvm_fep); >> >> > #endif >> >> > +static const char __initconst *warning_hvm_fep = >> >> > + "**********************************************\n" >> >> > + "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n" >> >> > + "******* This option is *ONLY* intended to aid testing of Xen.\n" >> >> > + "******* It has implications on the security of the system.\n" >> >> > + "******* Please *DO NOT* use this in production.\n" >> >> > + "**********************************************\n"; >> >> > + >> >> > >> >> >> >> Along with the same comment as on patch 2, here even more than >> >> there I wonder whether this string wouldn't better be a static local >> >> in hvm_enable() (or even the scope therein where warning_add() >> >> gets invoked). >> > >> > I would rather the text stays next to where the option is defined so >> > it is obvious to anyone who touches this area of code. >> >> Makes sense. But then - shouldn't it move inside the #ifndef? >> > > That would then require the warning_add() call to be wrapped in ifdef, > too. That looks a bit cumbersome to me. Ah, right. Well, never mind then - the compiler will (hopefully) take care of eliminating the unused string then. Jan
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index fed732c..8956e02 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -878,8 +878,12 @@ Recognized in debug builds of the hypervisor only. Allow use of the Forced Emulation Prefix in HVM guests, to allow emulation of arbitrary instructions. -This option is intended for development purposes, and is only available in -debug builds of the hypervisor. +This option is intended for development and testing purposes. + +*Warning* +As this feature opens up the instruction emulator to arbitrary +instruction from an HVM guest, don't use this in production system. No +security support is provided when this flag is set. ### hvm\_port80 > `= <boolean>` diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 73f79cc..c1e9279 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -59,6 +59,23 @@ config BIGMEM If unsure, say N. +config HVM_FEP + bool "HVM Forced Emulation Prefix support" if EXPERT = "y" + default DEBUG + ---help--- + + Compiles in a feature that allows HVM guest to arbitrarily + exercise the instruction emulator. + + This feature can only be enabled during boot time with + appropriate hypervisor command line option. Please read + hypervisor command line documentation before trying to use + this feature. + + This is strictly for testing purposes, and not appropriate + for use in production. + + If unsure, say N. endmenu source "common/Kconfig" diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 22f045e..52d66d4 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -37,6 +37,7 @@ #include <xen/mem_access.h> #include <xen/rangeset.h> #include <xen/vm_event.h> +#include <xen/warning.h> #include <asm/shadow.h> #include <asm/hap.h> #include <asm/current.h> @@ -97,9 +98,17 @@ boolean_param("hap", opt_hap_enabled); #ifndef opt_hvm_fep /* Permit use of the Forced Emulation Prefix in HVM guests */ -bool_t opt_hvm_fep; +bool_t __read_mostly opt_hvm_fep; boolean_param("hvm_fep", opt_hvm_fep); #endif +static const char __initconst *warning_hvm_fep = + "**********************************************\n" + "******* WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n" + "******* This option is *ONLY* intended to aid testing of Xen.\n" + "******* It has implications on the security of the system.\n" + "******* Please *DO NOT* use this in production.\n" + "**********************************************\n"; + /* Xen command-line option to enable altp2m */ static bool_t __initdata opt_altp2m_enabled = 0; @@ -182,6 +191,9 @@ static int __init hvm_enable(void) if ( !opt_altp2m_enabled ) hvm_funcs.altp2m_supported = 0; + if ( opt_hvm_fep ) + warning_add(warning_hvm_fep); + /* * Allow direct access to the PC debug ports 0x80 and 0xed (they are * often used for I/O delays, but the vmexits simply slow things down). @@ -3913,6 +3925,7 @@ void hvm_ud_intercept(struct cpu_user_regs *regs) { regs->eip += sizeof(sig); regs->eflags &= ~X86_EFLAGS_RF; + add_taint(TAINT_HVM_FEP); } } diff --git a/xen/common/kernel.c b/xen/common/kernel.c index dae7e35..5bf77aa 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -175,6 +175,7 @@ int __init parse_bool(const char *s) * 'M' - Machine had a machine check experience. * 'B' - System has hit bad_page. * 'C' - Console output is synchronous. + * 'H' - HVM forced emulation prefix is permitted. * * The string is overwritten by the next call to print_taint(). */ @@ -182,11 +183,12 @@ char *print_tainted(char *str) { if ( tainted ) { - snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c", + snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c", tainted & TAINT_UNSAFE_SMP ? 'S' : ' ', tainted & TAINT_MACHINE_CHECK ? 'M' : ' ', tainted & TAINT_BAD_PAGE ? 'B' : ' ', - tainted & TAINT_SYNC_CONSOLE ? 'C' : ' '); + tainted & TAINT_SYNC_CONSOLE ? 'C' : ' ', + tainted & TAINT_HVM_FEP ? 'H' : ' '); } else { diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index f486ee9..3c8aca8 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -27,7 +27,7 @@ #include <public/hvm/save.h> #include <xen/mm.h> -#ifndef NDEBUG +#ifdef CONFIG_HVM_FEP /* Permit use of the Forced Emulation Prefix in HVM guests */ extern bool_t opt_hvm_fep; #else diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 1c652bb..b1b0fb2 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -142,6 +142,7 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c); #define TAINT_BAD_PAGE (1<<2) #define TAINT_SYNC_CONSOLE (1<<3) #define TAINT_ERROR_INJECT (1<<4) +#define TAINT_HVM_FEP (1<<5) extern int tainted; #define TAINT_STRING_MAX_LEN 20 extern char *print_tainted(char *str);
Originally hvm_fep was guarded by NDEBUG, which means it was only available to debug builds. However there is value to have it for non-debug builds as well. User can use that to run tests in setup that replicates production setup. Make it clear with a sync_console style warning that this option can't be used in production setup. Update command line documentation accordingly. Finally mark Xen as tainted when this feature is enabled. Add a kconfig option under x86 to configure hvm_fep. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Doug Goldstein <cardoe@cardoe.com> v3: 1. Make config HVM_FEP an expert option and default to DEBUG. 2. Change some ifdefs 3. Update docs 4. Use the new warning infrastructure v2: 1. unsigned -> unsigned int 2. %d -> %u 3. Add spaces around "-" 4. Update warning message 5. Only taint hv when fep is used 6. Add kconfig option --- docs/misc/xen-command-line.markdown | 8 ++++++-- xen/arch/x86/Kconfig | 17 +++++++++++++++++ xen/arch/x86/hvm/hvm.c | 15 ++++++++++++++- xen/common/kernel.c | 6 ++++-- xen/include/asm-x86/hvm/hvm.h | 2 +- xen/include/xen/lib.h | 1 + 6 files changed, 43 insertions(+), 6 deletions(-)