Re: [PATCH] KVM: VMX: fixes for vmentry_l1d_flush module parameter

[Jack Wang]

Paolo Bonzini <pbonzini@xxxxxxxxxx> ä2018å8æ22æåä äå4:56åéï
>
> 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 <bsd@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: a7b9020b06ec6d7c3f3b0d4ef1a9eba12654f4f7
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
>  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},
>  };
>
>  #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
>
Tested-by: Jack Wang <jinpu.wang@xxxxxxxxxxxxxxxx>
Thanks,

Jack