On Wed, Aug 22, 2018 at 04:53:00PM +0200, Paolo Bonzini wrote:
> Two bug fixes:
>
> 1) missing entries in the l1d_param array; this can cause a host crash
> if an access attempts to reach the missing entry. Future-proof the get
> function against any overflows as well. However, the two entries
> VMENTER_L1D_FLUSH_EPT_DISABLED and VMENTER_L1D_FLUSH_NOT_REQUIRED must
> not be accepted by the parse function, so disable them there.
>
> 2) invalid values must be rejected even if the CPU does not have the
> bug, so test for them before checking boot_cpu_has(X86_BUG_L1TF)
>
> ... and a small refactoring, since the .cmd field is redundant with
> the index in the array.
>
> Reported-by: Bandan Das <[email protected]>
> Cc: [email protected]
> Fixes: a7b9020b06ec6d7c3f3b0d4ef1a9eba12654f4f7
> Signed-off-by: Paolo Bonzini <[email protected]>
Reviewed-by: Konrad Rzeszutek Wilk <[email protected]>
with one little nitpick
> ---
> arch/x86/kvm/vmx.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c76ca8c4befa..8dae47e7267a 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -198,12 +198,14 @@
>
> static const struct {
> const char *option;
> - enum vmx_l1d_flush_state cmd;
> + bool for_parse;
> } vmentry_l1d_param[] = {
> - {"auto", VMENTER_L1D_FLUSH_AUTO},
> - {"never", VMENTER_L1D_FLUSH_NEVER},
> - {"cond", VMENTER_L1D_FLUSH_COND},
> - {"always", VMENTER_L1D_FLUSH_ALWAYS},
> + [VMENTER_L1D_FLUSH_AUTO] = {"auto", true},
> + [VMENTER_L1D_FLUSH_NEVER] = {"never", true},
> + [VMENTER_L1D_FLUSH_COND] = {"cond", true},
> + [VMENTER_L1D_FLUSH_ALWAYS] = {"always", true},
> + [VMENTER_L1D_FLUSH_EPT_DISABLED] = {"EPT disabled", false},
> + [VMENTER_L1D_FLUSH_NOT_REQUIRED] = {"not required", false},
Ingo likes these to have tabs. Any chance you could do:
[VMENTER_L1D_FLUSH_NOT_REQUIRED] = {"not required", false},
or so?
> };
>
> #define L1D_CACHE_ORDER 4
> @@ -287,8 +289,9 @@ static int vmentry_l1d_flush_parse(const char *s)
>
> if (s) {
> for (i = 0; i < ARRAY_SIZE(vmentry_l1d_param); i++) {
> - if (sysfs_streq(s, vmentry_l1d_param[i].option))
> - return vmentry_l1d_param[i].cmd;
> + if (vmentry_l1d_param[i].for_parse &&
> + sysfs_streq(s, vmentry_l1d_param[i].option))
> + return i;
> }
> }
> return -EINVAL;
> @@ -298,13 +301,13 @@ static int vmentry_l1d_flush_set(const char *s, const
> struct kernel_param *kp)
> {
> int l1tf, ret;
>
> - if (!boot_cpu_has(X86_BUG_L1TF))
> - return 0;
> -
> l1tf = vmentry_l1d_flush_parse(s);
> if (l1tf < 0)
> return l1tf;
>
> + if (!boot_cpu_has(X86_BUG_L1TF))
> + return 0;
> +
> /*
> * Has vmx_init() run already? If not then this is the pre init
> * parameter parsing. In that case just store the value and let
> @@ -324,6 +327,9 @@ static int vmentry_l1d_flush_set(const char *s, const
> struct kernel_param *kp)
>
> static int vmentry_l1d_flush_get(char *s, const struct kernel_param *kp)
> {
> + if (WARN_ON_ONCE(l1tf_vmx_mitigation >= ARRAY_SIZE(vmentry_l1d_param)))
> + return sprintf(s, "???\n");
> +
> return sprintf(s, "%s\n",
> vmentry_l1d_param[l1tf_vmx_mitigation].option);
> }
>
> --
> 1.8.3.1
>
