[PATCH] target/i386: avoid theoretical leak on MCE injection

[PATCH] target/i386: avoid theoretical leak on MCE injection

[Paolo Bonzini]

g_strdup_printf is used twice to write to the same variable, which
can theoretically cause a leak.  In practice, it is extremely
unlikely that a guest is seeing a recursive MCE and has disabled
CR4.MCE between the first and the second error, but we can fix it
and we can also make a slight improvement on the logic: CR4.MCE=0
causes a triple fault even for a non-recursive machine check, so
let's place its test first.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/helper.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/target/i386/helper.c b/target/i386/helper.c
index 32fa21a7bb..f64379367d 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -908,16 +908,14 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
             return;
         }
 
-        if (recursive) {
-            need_reset = true;
-            msg = g_strdup_printf("CPU %d: Previous MCE still in progress, "
-                                  "raising triple fault", cs->cpu_index);
-        }
-
         if (!(cenv->cr[4] & CR4_MCE_MASK)) {
             need_reset = true;
             msg = g_strdup_printf("CPU %d: MCE capability is not enabled, "
                                   "raising triple fault", cs->cpu_index);
+        } else if (recursive) {
+            need_reset = true;
+            msg = g_strdup_printf("CPU %d: Previous MCE still in progress, "
+                                  "raising triple fault", cs->cpu_index);
         }
 
         if (need_reset) {
-- 
2.26.2

Re: [PATCH] target/i386: avoid theoretical leak on MCE injection

[Peter Maydell]

On Tue, 6 Oct 2020 at 08:54, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> g_strdup_printf is used twice to write to the same variable, which
> can theoretically cause a leak.  In practice, it is extremely
> unlikely that a guest is seeing a recursive MCE and has disabled
> CR4.MCE between the first and the second error, but we can fix it
> and we can also make a slight improvement on the logic: CR4.MCE=0
> causes a triple fault even for a non-recursive machine check, so
> let's place its test first.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/helper.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/target/i386/helper.c b/target/i386/helper.c
> index 32fa21a7bb..f64379367d 100644
> --- a/target/i386/helper.c
> +++ b/target/i386/helper.c
> @@ -908,16 +908,14 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
>              return;
>          }
>
> -        if (recursive) {
> -            need_reset = true;
> -            msg = g_strdup_printf("CPU %d: Previous MCE still in progress, "
> -                                  "raising triple fault", cs->cpu_index);
> -        }
> -
>          if (!(cenv->cr[4] & CR4_MCE_MASK)) {
>              need_reset = true;
>              msg = g_strdup_printf("CPU %d: MCE capability is not enabled, "
>                                    "raising triple fault", cs->cpu_index);
> +        } else if (recursive) {
> +            need_reset = true;
> +            msg = g_strdup_printf("CPU %d: Previous MCE still in progress, "
> +                                  "raising triple fault", cs->cpu_index);
>          }
>
>          if (need_reset) {

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Might be nice to have this in 5.2, given it is a coverity issue fix?

thanks
-- PMM