1. Patchew
  2. QEMU
  3. View series

[PATCH] target/i386: Check NULL monitor pointer when injecting MCE

Tao Su posted 1 patch 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240320052118.520378-1-tao1.su@linux.intel.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>
target/i386/helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] target/i386: Check NULL monitor pointer when injecting MCE
Posted by Tao Su 1 month, 1 week ago 
monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
may have a parameter with NULL monitor pointer. Check the monitor pointer
before calling monitor_puts().

Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Tao Su <tao1.su@linux.intel.com>
---
 target/i386/helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/i386/helper.c b/target/i386/helper.c
index 2070dd0dda..a9ff830a17 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
         if (need_reset) {
             emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
                                       recursive);
-            monitor_puts(params->mon, msg);
+            if (params->mon)
+                monitor_puts(params->mon, msg);
             qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
             qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
             return;
-- 
2.34.1
Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE
Posted by Markus Armbruster 1 month, 1 week ago 
Tao Su <tao1.su@linux.intel.com> writes:

> monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
> may have a parameter with NULL monitor pointer. Check the monitor pointer
> before calling monitor_puts().
>
> Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> ---
>  target/i386/helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/helper.c b/target/i386/helper.c
> index 2070dd0dda..a9ff830a17 100644
> --- a/target/i386/helper.c
> +++ b/target/i386/helper.c
> @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
>          if (need_reset) {
>              emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
>                                        recursive);
> -            monitor_puts(params->mon, msg);
> +            if (params->mon)
> +                monitor_puts(params->mon, msg);
>              qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
>              qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>              return;

Could instead revert the broken part of commit bf0c50d4aa85:

  -            monitor_puts(params->mon, msg);
  +            monitor_printf(params->mon, "%s", msg);
               qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);

Then the fact that we send the same message to monitor and log is again
more obvious.

Either way:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE
Posted by Tao Su 1 month, 1 week ago 
On Wed, Mar 20, 2024 at 07:02:46AM +0100, Markus Armbruster wrote:
> Tao Su <tao1.su@linux.intel.com> writes:
> 
> > monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
> > may have a parameter with NULL monitor pointer. Check the monitor pointer
> > before calling monitor_puts().
> >
> > Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> > Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> > ---
> >  target/i386/helper.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/i386/helper.c b/target/i386/helper.c
> > index 2070dd0dda..a9ff830a17 100644
> > --- a/target/i386/helper.c
> > +++ b/target/i386/helper.c
> > @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
> >          if (need_reset) {
> >              emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
> >                                        recursive);
> > -            monitor_puts(params->mon, msg);
> > +            if (params->mon)
> > +                monitor_puts(params->mon, msg);
> >              qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> >              qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> >              return;
> 
> Could instead revert the broken part of commit bf0c50d4aa85:
> 
>   -            monitor_puts(params->mon, msg);
>   +            monitor_printf(params->mon, "%s", msg);
>                qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> 
> Then the fact that we send the same message to monitor and log is again
> more obvious.

Good suggestion. I will send a v2 with this change.

> 
> Either way:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks for review!

>
Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE
Posted by Philippe Mathieu-Daudé 1 month, 1 week ago 
Hi Tao,

On 20/3/24 07:02, Markus Armbruster wrote:
> Tao Su <tao1.su@linux.intel.com> writes:
> 
>> monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
>> may have a parameter with NULL monitor pointer. Check the monitor pointer
>> before calling monitor_puts().
>>
>> Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
>> Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Signed-off-by: Tao Su <tao1.su@linux.intel.com>
>> ---
>>   target/i386/helper.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/helper.c b/target/i386/helper.c
>> index 2070dd0dda..a9ff830a17 100644
>> --- a/target/i386/helper.c
>> +++ b/target/i386/helper.c
>> @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
>>           if (need_reset) {
>>               emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
>>                                         recursive);
>> -            monitor_puts(params->mon, msg);
>> +            if (params->mon)

Missing braces, see QEMU coding style:
https://www.qemu.org/docs/master/devel/style.html#block-structure

>> +                monitor_puts(params->mon, msg);
>>               qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
>>               qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>               return;
> 
> Could instead revert the broken part of commit bf0c50d4aa85:
> 
>    -            monitor_puts(params->mon, msg);
>    +            monitor_printf(params->mon, "%s", msg);
>                 qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> 
> Then the fact that we send the same message to monitor and log is again
> more obvious.
> 
> Either way:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
>
Re: [PATCH] target/i386: Check NULL monitor pointer when injecting MCE
Posted by Tao Su 1 month, 1 week ago 
On Wed, Mar 20, 2024 at 08:17:36AM +0100, Philippe Mathieu-Daudé wrote:
> Hi Tao,
> 
> On 20/3/24 07:02, Markus Armbruster wrote:
> > Tao Su <tao1.su@linux.intel.com> writes:
> > 
> > > monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
> > > may have a parameter with NULL monitor pointer. Check the monitor pointer
> > > before calling monitor_puts().
> > > 
> > > Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> > > Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> > > ---
> > >   target/i386/helper.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/target/i386/helper.c b/target/i386/helper.c
> > > index 2070dd0dda..a9ff830a17 100644
> > > --- a/target/i386/helper.c
> > > +++ b/target/i386/helper.c
> > > @@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
> > >           if (need_reset) {
> > >               emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
> > >                                         recursive);
> > > -            monitor_puts(params->mon, msg);
> > > +            if (params->mon)
> 
> Missing braces, see QEMU coding style:
> https://www.qemu.org/docs/master/devel/style.html#block-structure

Yes, I prefer to revert the broken part.

Anyway, I got this point and will always pay attention to it, thanks for
reminding.

> 
> > > +                monitor_puts(params->mon, msg);
> > >               qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> > >               qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > >               return;
> > 
> > Could instead revert the broken part of commit bf0c50d4aa85:
> > 
> >    -            monitor_puts(params->mon, msg);
> >    +            monitor_printf(params->mon, "%s", msg);
> >                 qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
> > 
> > Then the fact that we send the same message to monitor and log is again
> > more obvious.
> > 
> > Either way:
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > 
> > 
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.