| Message ID | 20190822235052.3703-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] x86: Fix breakage of fw_cfg for 32-bit unit tests | expand |
> On Aug 22, 2019, at 4:50 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > Ensure the fw_cfg overrides are parsed prior consuming any of said > overrides. fwcfg_get_u() treats zero as a valid overide value, which > is slightly problematic since the overrides are in the .bss and thus > initialized to zero. > > Add a limit check when indexing fw_override so that future code doesn't > spontaneously explode. > > Cc: Nadav Amit <nadav.amit@gmail.com> > Fixes: 03b1e4570f967 ("x86: Support environments without test-devices") > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > lib/x86/fwcfg.c | 10 ++++++++-- > lib/x86/fwcfg.h | 2 -- > x86/cstart64.S | 2 -- > 3 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/lib/x86/fwcfg.c b/lib/x86/fwcfg.c > index d8d797f..06ef62c 100644 > --- a/lib/x86/fwcfg.c > +++ b/lib/x86/fwcfg.c > @@ -5,10 +5,11 @@ > static struct spinlock lock; > > static long fw_override[FW_CFG_MAX_ENTRY]; > +static bool fw_override_done; > > bool no_test_device; > > -void read_cfg_override(void) > +static void read_cfg_override(void) > { > const char *str; > int i; > @@ -26,6 +27,8 @@ void read_cfg_override(void) > > if ((str = getenv("TEST_DEVICE"))) > no_test_device = !atol(str); > + > + fw_override_done = true; > } > > static uint64_t fwcfg_get_u(uint16_t index, int bytes) > @@ -34,7 +37,10 @@ static uint64_t fwcfg_get_u(uint16_t index, int bytes) > uint8_t b; > int i; > > - if (fw_override[index] >= 0) > + if (!fw_override_done) > + read_cfg_override(); > + > + if (index < FW_CFG_MAX_ENTRY && fw_override[index] >= 0) > return fw_override[index]; How did that happen? I remember I tested this code with KVM.. Anyhow, Reviewed-by: Nadav Amit <nadav.amit@gmail.com> Thanks for fixing it.
On Thu, Aug 22, 2019 at 04:55:14PM -0700, Nadav Amit wrote: > > On Aug 22, 2019, at 4:50 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > Ensure the fw_cfg overrides are parsed prior consuming any of said > > overrides. fwcfg_get_u() treats zero as a valid overide value, which > > is slightly problematic since the overrides are in the .bss and thus > > initialized to zero. > > > > Add a limit check when indexing fw_override so that future code doesn't > > spontaneously explode. > > > > Cc: Nadav Amit <nadav.amit@gmail.com> > > Fixes: 03b1e4570f967 ("x86: Support environments without test-devices") > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > lib/x86/fwcfg.c | 10 ++++++++-- > > lib/x86/fwcfg.h | 2 -- > > x86/cstart64.S | 2 -- > > 3 files changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/lib/x86/fwcfg.c b/lib/x86/fwcfg.c > > index d8d797f..06ef62c 100644 > > --- a/lib/x86/fwcfg.c > > +++ b/lib/x86/fwcfg.c > > @@ -5,10 +5,11 @@ > > static struct spinlock lock; > > > > static long fw_override[FW_CFG_MAX_ENTRY]; > > +static bool fw_override_done; > > > > bool no_test_device; > > > > -void read_cfg_override(void) > > +static void read_cfg_override(void) > > { > > const char *str; > > int i; > > @@ -26,6 +27,8 @@ void read_cfg_override(void) > > > > if ((str = getenv("TEST_DEVICE"))) > > no_test_device = !atol(str); > > + > > + fw_override_done = true; > > } > > > > static uint64_t fwcfg_get_u(uint16_t index, int bytes) > > @@ -34,7 +37,10 @@ static uint64_t fwcfg_get_u(uint16_t index, int bytes) > > uint8_t b; > > int i; > > > > - if (fw_override[index] >= 0) > > + if (!fw_override_done) > > + read_cfg_override(); > > + > > + if (index < FW_CFG_MAX_ENTRY && fw_override[index] >= 0) > > return fw_override[index]; > > How did that happen? I remember I tested this code with KVM.. It only breaks 32-bit KVM.
diff --git a/lib/x86/fwcfg.c b/lib/x86/fwcfg.c index d8d797f..06ef62c 100644 --- a/lib/x86/fwcfg.c +++ b/lib/x86/fwcfg.c @@ -5,10 +5,11 @@ static struct spinlock lock; static long fw_override[FW_CFG_MAX_ENTRY]; +static bool fw_override_done; bool no_test_device; -void read_cfg_override(void) +static void read_cfg_override(void) { const char *str; int i; @@ -26,6 +27,8 @@ void read_cfg_override(void) if ((str = getenv("TEST_DEVICE"))) no_test_device = !atol(str); + + fw_override_done = true; } static uint64_t fwcfg_get_u(uint16_t index, int bytes) @@ -34,7 +37,10 @@ static uint64_t fwcfg_get_u(uint16_t index, int bytes) uint8_t b; int i; - if (fw_override[index] >= 0) + if (!fw_override_done) + read_cfg_override(); + + if (index < FW_CFG_MAX_ENTRY && fw_override[index] >= 0) return fw_override[index]; spin_lock(&lock); diff --git a/lib/x86/fwcfg.h b/lib/x86/fwcfg.h index 88dc7a7..2f17461 100644 --- a/lib/x86/fwcfg.h +++ b/lib/x86/fwcfg.h @@ -36,8 +36,6 @@ extern bool no_test_device; -void read_cfg_override(void); - static inline bool test_device_enabled(void) { return !no_test_device; diff --git a/x86/cstart64.S b/x86/cstart64.S index 23c1bd4..d4e4652 100644 --- a/x86/cstart64.S +++ b/x86/cstart64.S @@ -254,8 +254,6 @@ start64: mov %rax, __args(%rip) call __setup_args - /* Read the configuration before running smp_init */ - call read_cfg_override call smp_init call enable_x2apic
Ensure the fw_cfg overrides are parsed prior consuming any of said overrides. fwcfg_get_u() treats zero as a valid overide value, which is slightly problematic since the overrides are in the .bss and thus initialized to zero. Add a limit check when indexing fw_override so that future code doesn't spontaneously explode. Cc: Nadav Amit <nadav.amit@gmail.com> Fixes: 03b1e4570f967 ("x86: Support environments without test-devices") Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- lib/x86/fwcfg.c | 10 ++++++++-- lib/x86/fwcfg.h | 2 -- x86/cstart64.S | 2 -- 3 files changed, 8 insertions(+), 6 deletions(-)