| Message ID | 20201006074829.488968-1-pbonzini@redhat.com |
|---|---|
| State | Accepted |
| Commit | 42ccce19818e4e8fb55026f50b20d533cccc48f6 |
| Headers | show |
| Series | target/i386: avoid theoretical leak on MCE injection | expand |
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
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) {
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(-)