QEMU

null-ptr dereference in ich9_apm_ctrl_changed

Bug #1878645 reported by Alexander Bulekov
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
QEMU
Invalid
Undecided
Unassigned

Bug Description

Hello,
While fuzzing, I found an input which triggers a NULL pointer dereference in
tcg_handle_interrupt. It seems the culprint is a "cpu" pointer - maybe this bug
is specific to QTest?

==23862==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000000b4 (pc 0x55b9dc7c9dce bp 0x7ffc346a0900 sp 0x7ffc346a0880 T0)
==23862==The signal is caused by a READ memory access.
==23862==Hint: address points to the zero page.
    #0 0x55b9dc7c9dce in tcg_handle_interrupt /home/alxndr/Development/qemu/accel/tcg/tcg-all.c:57:21
    #1 0x55b9dc904799 in cpu_interrupt /home/alxndr/Development/qemu/include/hw/core/cpu.h:872:5
    #2 0x55b9dc9085e8 in ich9_apm_ctrl_changed /home/alxndr/Development/qemu/hw/isa/lpc_ich9.c:442:13
    #3 0x55b9dd19cdc8 in apm_ioport_writeb /home/alxndr/Development/qemu/hw/isa/apm.c:50:13
    #4 0x55b9dc73f8b4 in memory_region_write_accessor /home/alxndr/Development/qemu/memory.c:483:5
    #5 0x55b9dc73f289 in access_with_adjusted_size /home/alxndr/Development/qemu/memory.c:544:18
    #6 0x55b9dc73ddf5 in memory_region_dispatch_write /home/alxndr/Development/qemu/memory.c:1476:16
    #7 0x55b9dc577bf3 in flatview_write_continue /home/alxndr/Development/qemu/exec.c:3137:23
    #8 0x55b9dc567ad8 in flatview_write /home/alxndr/Development/qemu/exec.c:3177:14
    #9 0x55b9dc567608 in address_space_write /home/alxndr/Development/qemu/exec.c:3268:18
    #10 0x55b9dc723fe7 in cpu_outb /home/alxndr/Development/qemu/ioport.c:60:5
    #11 0x55b9dc72d3c0 in qtest_process_command /home/alxndr/Development/qemu/qtest.c:392:13
    #12 0x55b9dc72b186 in qtest_process_inbuf /home/alxndr/Development/qemu/qtest.c:710:9
    #13 0x55b9dc72a8b3 in qtest_read /home/alxndr/Development/qemu/qtest.c:722:5
    #14 0x55b9ddc6e60b in qemu_chr_be_write_impl /home/alxndr/Development/qemu/chardev/char.c:183:9
    #15 0x55b9ddc6e75a in qemu_chr_be_write /home/alxndr/Development/qemu/chardev/char.c:195:9
    #16 0x55b9ddc77979 in fd_chr_read /home/alxndr/Development/qemu/chardev/char-fd.c:68:9
    #17 0x55b9ddcff0e9 in qio_channel_fd_source_dispatch /home/alxndr/Development/qemu/io/channel-watch.c:84:12
    #18 0x7f7161eac897 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4e897)
    #19 0x55b9ddebcb84 in glib_pollfds_poll /home/alxndr/Development/qemu/util/main-loop.c:219:9
    #20 0x55b9ddebb57d in os_host_main_loop_wait /home/alxndr/Development/qemu/util/main-loop.c:242:5
    #21 0x55b9ddebb176 in main_loop_wait /home/alxndr/Development/qemu/util/main-loop.c:518:11
    #22 0x55b9dcb4bd1d in qemu_main_loop /home/alxndr/Development/qemu/softmmu/vl.c:1664:9
    #23 0x55b9ddd1629c in main /home/alxndr/Development/qemu/softmmu/main.c:49:5
    #24 0x7f7160a5ce0a in __libc_start_main /build/glibc-GwnBeO/glibc-2.30/csu/../csu/libc-start.c:308:16
    #25 0x55b9dc49c819 in _start (/home/alxndr/Development/qemu/build/i386-softmmu/qemu-system-i386+0xc9c819)

I can reproduce this in qemu 5.0 built with AddressSanitizer using these qtest commands:

cat << EOF | ./qemu-system-i386 \
-qtest stdio -nographic -monitor none -serial none \
-M pc-q35-5.0
outl 0xcf8 0x8400f841
outl 0xcfc 0xaa215d6d
outl 0x6d30 0x2ef8ffbe
outb 0xb2 0x20
EOF

Please let me know if I can provide any further info.
-Alex

Revision history for this message
Alexander Bulekov (a1xndr) wrote :

I don't think this is a qtest-specific error:
cat << EOF| qemu-system-i386 -M q35 -nographic -serial none -monitor stdio
o/4 0xcf8 0x8400f841
o/4 0xcfc 0xaa215d6d
o/4 0x6d30 0x2ef8ffbe
o/1 0xb2 0x20
EOF

...
Segmentation fault

Philippe Mathieu-Daudé (philmd)
summary: - null-ptr dereference in tcg_handle_interrupt
+ null-ptr dereference in ich9_apm_ctrl_changed
Revision history for this message
Alex Bennée (ajbennee) wrote : Re: [Bug 1878645] Re: null-ptr dereference in tcg_handle_interrupt

Alexander Bulekov <email address hidden> writes:

> I don't think this is a qtest-specific error:
> cat << EOF| qemu-system-i386 -M q35 -nographic -serial none -monitor stdio
> o/4 0xcf8 0x8400f841
> o/4 0xcfc 0xaa215d6d
> o/4 0x6d30 0x2ef8ffbe
> o/1 0xb2 0x20
> EOF
>
> ...
> Segmentation fault

Both this and the qtest have the same problem of depending on
current_cpu which is a TLS variable which will never be correct from the
qtest or monitor context. There are only a few other cases.

sun4m:cpu_halt_signal does:

    if (level && current_cpu) {
        cpu_interrupt(current_cpu, CPU_INTERRUPT_HALT);
    }

pxa2xx:pxa2xx_pwrmode_write does a bare:

    /* Suspend */
    cpu_interrupt(current_cpu, CPU_INTERRUPT_HALT);

but given the context has a CPUARMState *env it could arguably use that
to derive current_cpu but as it's only triggered by a system register
write you can't actually trigger from a monitor/qtest command.

I would suggest either:

        } else if (current_cpu) {
            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
        }

or possibly:

        } else {
            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
        }

if you really care about triggering a real IRQ from outside the CPU context.

--
Alex Bennée

Revision history for this message
Alexander Bulekov (a1xndr) wrote :

On 200629 2000, Alex Bennée wrote:
>
> Alexander Bulekov <email address hidden> writes:
>
> > I don't think this is a qtest-specific error:
> > cat << EOF| qemu-system-i386 -M q35 -nographic -serial none -monitor stdio
> > o/4 0xcf8 0x8400f841
> > o/4 0xcfc 0xaa215d6d
> > o/4 0x6d30 0x2ef8ffbe
> > o/1 0xb2 0x20
> > EOF
> >
> > ...
> > Segmentation fault
>
> Both this and the qtest have the same problem of depending on
> current_cpu which is a TLS variable which will never be correct from the
> qtest or monitor context. There are only a few other cases.

Ah that makes sense. It probably isn't a real issue, but I'll send
patches with the changes you suggested below.
Thank you

> sun4m:cpu_halt_signal does:
>
> if (level && current_cpu) {
> cpu_interrupt(current_cpu, CPU_INTERRUPT_HALT);
> }
>
> pxa2xx:pxa2xx_pwrmode_write does a bare:
>
> /* Suspend */
> cpu_interrupt(current_cpu, CPU_INTERRUPT_HALT);
>
> but given the context has a CPUARMState *env it could arguably use that
> to derive current_cpu but as it's only triggered by a system register
> write you can't actually trigger from a monitor/qtest command.
>
> I would suggest either:
>
> } else if (current_cpu) {
> cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> }
>
> or possibly:
>
> } else {
> cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
> }
>
> if you really care about triggering a real IRQ from outside the CPU context.
>
> --
> Alex Bennée
>

Revision history for this message
Alex Bennée (ajbennee) wrote : [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ

It's possible to trigger this function from qtest/monitor at which
point current_cpu won't point at the right place. Check it and
fall back to first_cpu if it's NULL.

Signed-off-by: Alex Bennée <email address hidden>
Cc: Bug 1878645 <email address hidden>
---
 hw/isa/lpc_ich9.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index cd6e169d47a..791c878eb0b 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
                 cpu_interrupt(cs, CPU_INTERRUPT_SMI);
             }
         } else {
- cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+ cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
         }
     }
 }
--
2.20.1

Revision history for this message
Philippe Mathieu-Daudé (philmd) wrote : Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ

On 7/1/20 3:56 PM, Alex Bennée wrote:
> It's possible to trigger this function from qtest/monitor at which
> point current_cpu won't point at the right place. Check it and
> fall back to first_cpu if it's NULL.
>
> Signed-off-by: Alex Bennée <email address hidden>
> Cc: Bug 1878645 <email address hidden>
> ---
> hw/isa/lpc_ich9.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index cd6e169d47a..791c878eb0b 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
> cpu_interrupt(cs, CPU_INTERRUPT_SMI);
> }
> } else {
> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> + cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);

I'm not sure this change anything, as first_cpu is NULL when using
qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
GDB connection segfault caused by empty machines").

> }
> }
> }
>

Revision history for this message
Alex Bennée (ajbennee) wrote :

Philippe Mathieu-Daudé <email address hidden> writes:

> On 7/1/20 3:56 PM, Alex Bennée wrote:
>> It's possible to trigger this function from qtest/monitor at which
>> point current_cpu won't point at the right place. Check it and
>> fall back to first_cpu if it's NULL.
>>
>> Signed-off-by: Alex Bennée <email address hidden>
>> Cc: Bug 1878645 <email address hidden>
>> ---
>> hw/isa/lpc_ich9.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index cd6e169d47a..791c878eb0b 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>> cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>> }
>> } else {
>> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>> + cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
>
> I'm not sure this change anything, as first_cpu is NULL when using
> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
> GDB connection segfault caused by empty machines").

Good point - anyway feel free to ignore - it shouldn't have been in this
series. It was just some random experimentation I was doing when looking
at that bug.

--
Alex Bennée

Revision history for this message
Philippe Mathieu-Daudé (philmd) wrote :

On 7/1/20 6:40 PM, Alex Bennée wrote:
>
> Philippe Mathieu-Daudé <email address hidden> writes:
>
>> On 7/1/20 3:56 PM, Alex Bennée wrote:
>>> It's possible to trigger this function from qtest/monitor at which
>>> point current_cpu won't point at the right place. Check it and
>>> fall back to first_cpu if it's NULL.
>>>
>>> Signed-off-by: Alex Bennée <email address hidden>
>>> Cc: Bug 1878645 <email address hidden>
>>> ---
>>> hw/isa/lpc_ich9.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>> index cd6e169d47a..791c878eb0b 100644
>>> --- a/hw/isa/lpc_ich9.c
>>> +++ b/hw/isa/lpc_ich9.c
>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>>> cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>> }
>>> } else {
>>> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>>> + cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
>>
>> I'm not sure this change anything, as first_cpu is NULL when using
>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
>> GDB connection segfault caused by empty machines").
>
> Good point - anyway feel free to ignore - it shouldn't have been in this
> series. It was just some random experimentation I was doing when looking
> at that bug.

See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
crashing") for a similar approach, but here I was thinking about
a more generic fix, not very intrusive:

-- >8 --
diff --git a/hw/isa/apm.c b/hw/isa/apm.c
index bce266b957..809afeb3e4 100644
--- a/hw/isa/apm.c
+++ b/hw/isa/apm.c
@@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
addr, uint64_t val,
     if (addr == 0) {
         apm->apmc = val;

- if (apm->callback) {
+ if (apm->callback && !qtest_enabled()) {
             (apm->callback)(val, apm->arg);
         }
     } else {
---

Revision history for this message
Alex Bennée (ajbennee) wrote :

Philippe Mathieu-Daudé <email address hidden> writes:

> On 7/1/20 6:40 PM, Alex Bennée wrote:
>>
>> Philippe Mathieu-Daudé <email address hidden> writes:
>>
>>> On 7/1/20 3:56 PM, Alex Bennée wrote:
>>>> It's possible to trigger this function from qtest/monitor at which
>>>> point current_cpu won't point at the right place. Check it and
>>>> fall back to first_cpu if it's NULL.
>>>>
>>>> Signed-off-by: Alex Bennée <email address hidden>
>>>> Cc: Bug 1878645 <email address hidden>
>>>> ---
>>>> hw/isa/lpc_ich9.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>>> index cd6e169d47a..791c878eb0b 100644
>>>> --- a/hw/isa/lpc_ich9.c
>>>> +++ b/hw/isa/lpc_ich9.c
>>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>>>> cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>>> }
>>>> } else {
>>>> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>>>> + cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
>>>
>>> I'm not sure this change anything, as first_cpu is NULL when using
>>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
>>> GDB connection segfault caused by empty machines").
>>
>> Good point - anyway feel free to ignore - it shouldn't have been in this
>> series. It was just some random experimentation I was doing when looking
>> at that bug.
>
> See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
> crashing") for a similar approach, but here I was thinking about
> a more generic fix, not very intrusive:
>
> -- >8 --
> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
> index bce266b957..809afeb3e4 100644
> --- a/hw/isa/apm.c
> +++ b/hw/isa/apm.c
> @@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
> addr, uint64_t val,
> if (addr == 0) {
> apm->apmc = val;
>
> - if (apm->callback) {
> + if (apm->callback && !qtest_enabled()) {
> (apm->callback)(val, apm->arg);
> }

But the other failure mode reported on the bug thread was via the
monitor - so I'm not sure just checking for qtest catches that.

> } else {
> ---

--
Alex Bennée

Revision history for this message
Philippe Mathieu-Daudé (philmd) wrote :

+Paolo

On 7/1/20 7:09 PM, Alex Bennée wrote:
> Philippe Mathieu-Daudé <email address hidden> writes:
>> On 7/1/20 6:40 PM, Alex Bennée wrote:
>>> Philippe Mathieu-Daudé <email address hidden> writes:
>>>
>>>> On 7/1/20 3:56 PM, Alex Bennée wrote:
>>>>> It's possible to trigger this function from qtest/monitor at which
>>>>> point current_cpu won't point at the right place. Check it and
>>>>> fall back to first_cpu if it's NULL.
>>>>>
>>>>> Signed-off-by: Alex Bennée <email address hidden>
>>>>> Cc: Bug 1878645 <email address hidden>
>>>>> ---
>>>>> hw/isa/lpc_ich9.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>>>> index cd6e169d47a..791c878eb0b 100644
>>>>> --- a/hw/isa/lpc_ich9.c
>>>>> +++ b/hw/isa/lpc_ich9.c
>>>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>>>>> cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>>>> }
>>>>> } else {
>>>>> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>>>>> + cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
>>>>
>>>> I'm not sure this change anything, as first_cpu is NULL when using
>>>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
>>>> GDB connection segfault caused by empty machines").
>>>
>>> Good point - anyway feel free to ignore - it shouldn't have been in this
>>> series. It was just some random experimentation I was doing when looking
>>> at that bug.
>>
>> See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
>> crashing") for a similar approach, but here I was thinking about
>> a more generic fix, not very intrusive:
>>
>> -- >8 --
>> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
>> index bce266b957..809afeb3e4 100644
>> --- a/hw/isa/apm.c
>> +++ b/hw/isa/apm.c
>> @@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
>> addr, uint64_t val,
>> if (addr == 0) {
>> apm->apmc = val;
>>
>> - if (apm->callback) {
>> + if (apm->callback && !qtest_enabled()) {
>> (apm->callback)(val, apm->arg);
>> }
>
> But the other failure mode reported on the bug thread was via the
> monitor - so I'm not sure just checking for qtest catches that.

Ah indeed.

in exec.c:

/* current CPU in the current thread. It is only valid inside
   cpu_exec() */
__thread CPUState *current_cpu;

Maybe we shouldn't use current_cpu out of exec.c...

Revision history for this message
Philippe Mathieu-Daudé (philmd) wrote :

On 7/1/20 7:34 PM, Philippe Mathieu-Daudé wrote:
> +Paolo
>
> On 7/1/20 7:09 PM, Alex Bennée wrote:
>> Philippe Mathieu-Daudé <email address hidden> writes:
>>> On 7/1/20 6:40 PM, Alex Bennée wrote:
>>>> Philippe Mathieu-Daudé <email address hidden> writes:
>>>>
>>>>> On 7/1/20 3:56 PM, Alex Bennée wrote:
>>>>>> It's possible to trigger this function from qtest/monitor at which
>>>>>> point current_cpu won't point at the right place. Check it and
>>>>>> fall back to first_cpu if it's NULL.
>>>>>>
>>>>>> Signed-off-by: Alex Bennée <email address hidden>
>>>>>> Cc: Bug 1878645 <email address hidden>
>>>>>> ---
>>>>>> hw/isa/lpc_ich9.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>>>>> index cd6e169d47a..791c878eb0b 100644
>>>>>> --- a/hw/isa/lpc_ich9.c
>>>>>> +++ b/hw/isa/lpc_ich9.c
>>>>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>>>>>> cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>>>>> }
>>>>>> } else {
>>>>>> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>>>>>> + cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
>>>>>
>>>>> I'm not sure this change anything, as first_cpu is NULL when using
>>>>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
>>>>> GDB connection segfault caused by empty machines").
>>>>
>>>> Good point - anyway feel free to ignore - it shouldn't have been in this
>>>> series. It was just some random experimentation I was doing when looking
>>>> at that bug.
>>>
>>> See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
>>> crashing") for a similar approach, but here I was thinking about
>>> a more generic fix, not very intrusive:
>>>
>>> -- >8 --
>>> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
>>> index bce266b957..809afeb3e4 100644
>>> --- a/hw/isa/apm.c
>>> +++ b/hw/isa/apm.c
>>> @@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
>>> addr, uint64_t val,
>>> if (addr == 0) {
>>> apm->apmc = val;
>>>
>>> - if (apm->callback) {
>>> + if (apm->callback && !qtest_enabled()) {
>>> (apm->callback)(val, apm->arg);
>>> }
>>
>> But the other failure mode reported on the bug thread was via the
>> monitor - so I'm not sure just checking for qtest catches that.
>
> Ah indeed.
>
> in exec.c:
>
> /* current CPU in the current thread. It is only valid inside
> cpu_exec() */
> __thread CPUState *current_cpu;
>
> Maybe we shouldn't use current_cpu out of exec.c...

I meant, out of cpu_exec(), a cpu thread. Here we access it
from an I/O thread.

Revision history for this message
Philippe Mathieu-Daudé (philmd) wrote :
Download full text (3.5 KiB)

+MST/Igor for ICH9

On 7/1/20 7:37 PM, Philippe Mathieu-Daudé wrote:
> On 7/1/20 7:34 PM, Philippe Mathieu-Daudé wrote:
>> +Paolo
>>
>> On 7/1/20 7:09 PM, Alex Bennée wrote:
>>> Philippe Mathieu-Daudé <email address hidden> writes:
>>>> On 7/1/20 6:40 PM, Alex Bennée wrote:
>>>>> Philippe Mathieu-Daudé <email address hidden> writes:
>>>>>
>>>>>> On 7/1/20 3:56 PM, Alex Bennée wrote:
>>>>>>> It's possible to trigger this function from qtest/monitor at which
>>>>>>> point current_cpu won't point at the right place. Check it and
>>>>>>> fall back to first_cpu if it's NULL.
>>>>>>>
>>>>>>> Signed-off-by: Alex Bennée <email address hidden>
>>>>>>> Cc: Bug 1878645 <email address hidden>
>>>>>>> ---
>>>>>>> hw/isa/lpc_ich9.c | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>>>>>> index cd6e169d47a..791c878eb0b 100644
>>>>>>> --- a/hw/isa/lpc_ich9.c
>>>>>>> +++ b/hw/isa/lpc_ich9.c
>>>>>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>>>>>>> cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>>>>>> }
>>>>>>> } else {
>>>>>>> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>>>>>>> + cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
>>>>>>
>>>>>> I'm not sure this change anything, as first_cpu is NULL when using
>>>>>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
>>>>>> GDB connection segfault caused by empty machines").
>>>>>
>>>>> Good point - anyway feel free to ignore - it shouldn't have been in this
>>>>> series. It was just some random experimentation I was doing when looking
>>>>> at that bug.
>>>>
>>>> See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
>>>> crashing") for a similar approach, but here I was thinking about
>>>> a more generic fix, not very intrusive:
>>>>
>>>> -- >8 --
>>>> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
>>>> index bce266b957..809afeb3e4 100644
>>>> --- a/hw/isa/apm.c
>>>> +++ b/hw/isa/apm.c
>>>> @@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
>>>> addr, uint64_t val,
>>>> if (addr == 0) {
>>>> apm->apmc = val;
>>>>
>>>> - if (apm->callback) {
>>>> + if (apm->callback && !qtest_enabled()) {
>>>> (apm->callback)(val, apm->arg);
>>>> }
>>>
>>> But the other failure mode reported on the bug thread was via the
>>> monitor - so I'm not sure just checking for qtest catches that.
>>
>> Ah indeed.
>>
>> in exec.c:
>>
>> /* current CPU in the current thread. It is only valid inside
>> cpu_exec() */
>> __thread CPUState *current_cpu;
>>
>> Maybe we shouldn't use current_cpu out of exec.c...
>
> I meant, out of cpu_exec(), a cpu thread. Here we access it
> from an I/O thread.

ARM and S390X use:

hw/arm/boot.c:460: ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
hw/arm/virt.c:331: armcpu = ARM_CPU(qemu_get_cpu(0));
hw/arm/virt.c:549: armcpu = ARM_CPU(qemu_get_cpu(0));
hw/cpu/a15mpcore.c:69: cpuobj = OBJECT(qemu_get_cpu(0));
hw/cpu/a9mpcore.c:76: cpuobj = OBJECT(qemu_get_cpu(0));
target/s390x/cpu_models.c:155: cpu = ...

Read more...

Revision history for this message
Philippe Mathieu-Daudé (philmd) wrote :
Download full text (3.8 KiB)

On 7/1/20 7:48 PM, Philippe Mathieu-Daudé wrote:
> +MST/Igor for ICH9
>
> On 7/1/20 7:37 PM, Philippe Mathieu-Daudé wrote:
>> On 7/1/20 7:34 PM, Philippe Mathieu-Daudé wrote:
>>> +Paolo
>>>
>>> On 7/1/20 7:09 PM, Alex Bennée wrote:
>>>> Philippe Mathieu-Daudé <email address hidden> writes:
>>>>> On 7/1/20 6:40 PM, Alex Bennée wrote:
>>>>>> Philippe Mathieu-Daudé <email address hidden> writes:
>>>>>>
>>>>>>> On 7/1/20 3:56 PM, Alex Bennée wrote:
>>>>>>>> It's possible to trigger this function from qtest/monitor at which
>>>>>>>> point current_cpu won't point at the right place. Check it and
>>>>>>>> fall back to first_cpu if it's NULL.
>>>>>>>>
>>>>>>>> Signed-off-by: Alex Bennée <email address hidden>
>>>>>>>> Cc: Bug 1878645 <email address hidden>
>>>>>>>> ---
>>>>>>>> hw/isa/lpc_ich9.c | 2 +-
>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>>>>>>>> index cd6e169d47a..791c878eb0b 100644
>>>>>>>> --- a/hw/isa/lpc_ich9.c
>>>>>>>> +++ b/hw/isa/lpc_ich9.c
>>>>>>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
>>>>>>>> cpu_interrupt(cs, CPU_INTERRUPT_SMI);
>>>>>>>> }
>>>>>>>> } else {
>>>>>>>> - cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
>>>>>>>> + cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
>>>>>>>
>>>>>>> I'm not sure this change anything, as first_cpu is NULL when using
>>>>>>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
>>>>>>> GDB connection segfault caused by empty machines").
>>>>>>
>>>>>> Good point - anyway feel free to ignore - it shouldn't have been in this
>>>>>> series. It was just some random experimentation I was doing when looking
>>>>>> at that bug.
>>>>>
>>>>> See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
>>>>> crashing") for a similar approach, but here I was thinking about
>>>>> a more generic fix, not very intrusive:
>>>>>
>>>>> -- >8 --
>>>>> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
>>>>> index bce266b957..809afeb3e4 100644
>>>>> --- a/hw/isa/apm.c
>>>>> +++ b/hw/isa/apm.c
>>>>> @@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
>>>>> addr, uint64_t val,
>>>>> if (addr == 0) {
>>>>> apm->apmc = val;
>>>>>
>>>>> - if (apm->callback) {
>>>>> + if (apm->callback && !qtest_enabled()) {
>>>>> (apm->callback)(val, apm->arg);
>>>>> }
>>>>
>>>> But the other failure mode reported on the bug thread was via the
>>>> monitor - so I'm not sure just checking for qtest catches that.
>>>
>>> Ah indeed.
>>>
>>> in exec.c:
>>>
>>> /* current CPU in the current thread. It is only valid inside
>>> cpu_exec() */
>>> __thread CPUState *current_cpu;
>>>
>>> Maybe we shouldn't use current_cpu out of exec.c...
>>
>> I meant, out of cpu_exec(), a cpu thread. Here we access it
>> from an I/O thread.

Ah! we are in the monitor thread... It makes sense there is not
current_cpu assigned :)

>
> ARM and S390X use:
>
> hw/arm/boot.c:460: ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> hw/arm/virt.c:331: armcpu = ARM_CPU(qemu_get_cpu(0))...

Read more...

Revision history for this message
Philippe Mathieu-Daudé (philmd) wrote : [RFC PATCH] cpus: Initialize current_cpu with the first vCPU created
Download full text (3.3 KiB)

We can run I/O access with the 'i' or 'o' HMP commands in the
monitor. These commands are expected to run on a vCPU. The
monitor is not a vCPU thread. To avoid crashing, initialize
the 'current_cpu' variable with the first vCPU created. The
command executed on the monitor will end using it.

This fixes:

  $ cat << EOF| qemu-system-i386 -M q35 -nographic -serial none -monitor stdio
  o/4 0xcf8 0x8400f841
  o/4 0xcfc 0xaa215d6d
  o/4 0x6d30 0x2ef8ffbe
  o/1 0xb2 0x20
  EOF
  Segmentation fault (core dumped)

  Thread 1 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
  0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57
  57 old_mask = cpu->interrupt_request;
  (gdb) bt
  #0 0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57
  #1 0x00005555558ed7d2 in cpu_interrupt (cpu=0x0, mask=64) at include/hw/core/cpu.h:877
  #2 0x00005555558ee776 in ich9_apm_ctrl_changed (val=32, arg=0x555556e2ff50) at hw/isa/lpc_ich9.c:442
  #3 0x0000555555b47f96 in apm_ioport_writeb (opaque=0x555556e308c0, addr=0, val=32, size=1) at hw/isa/apm.c:44
  #4 0x0000555555879931 in memory_region_write_accessor (mr=0x555556e308e0, addr=0, value=0x7fffffffb9f8, size=1, shift=0, mask=255, attrs=...) at memory.c:483
  #5 0x0000555555879b5a in access_with_adjusted_size (addr=0, value=0x7fffffffb9f8, size=4, access_size_min=1, access_size_max=1, access_fn=
      0x55555587984e <memory_region_write_accessor>, mr=0x555556e308e0, attrs=...) at memory.c:544
  #6 0x000055555587ca32 in memory_region_dispatch_write (mr=0x555556e308e0, addr=0, data=32, op=MO_32, attrs=...) at memory.c:1465
  #7 0x000055555581b7e9 in flatview_write_continue (fv=0x55555698a790, addr=178, attrs=..., ptr=0x7fffffffbb84, len=4, addr1=0, l=4, mr=0x555556e308e0) at exec.c:3198
  #8 0x000055555581b92e in flatview_write (fv=0x55555698a790, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3238
  #9 0x000055555581bc81 in address_space_write (as=0x555556441220 <address_space_io>, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3329
  #10 0x0000555555873f08 in cpu_outl (addr=178, val=32) at ioport.c:80
  #11 0x000055555598a26a in hmp_ioport_write (mon=0x5555567621b0, qdict=0x555557702600) at monitor/misc.c:937
  #12 0x0000555555c9c5a5 in handle_hmp_command (mon=0x5555567621b0, cmdline=0x55555676aae1 "/1 0xb2 0x20") at monitor/hmp.c:1082
  #13 0x0000555555c99e02 in monitor_command_cb (opaque=0x5555567621b0, cmdline=0x55555676aae0 "o/1 0xb2 0x20", readline_opaque=0x0) at monitor/hmp.c:47
                            ^
    HMP command from monitor

Reported-by: Alexander Bulekov <email address hidden>
Buglink: https://bugs.launchpad.net/qemu/+bug/1878645
Signed-off-by: Philippe Mathieu-Daudé <email address hidden>
---
Cc: Bug 1878645 <email address hidden>

RFC because I believe the correct fix is to NOT use current_cpu
out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU.
---
 cpus.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/cpus.c b/cpus.c
index 41d1c5099f..1f6f43d221 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2106,6 +2106,9 @@ void qemu_init_vcpu(CPUState *cpu)
 {
     MachineState...

Read more...

Revision history for this message
Alexander Bulekov (a1xndr) wrote :
Download full text (3.7 KiB)

On 200701 2021, Philippe Mathieu-Daudé wrote:
> We can run I/O access with the 'i' or 'o' HMP commands in the
> monitor. These commands are expected to run on a vCPU. The
> monitor is not a vCPU thread. To avoid crashing, initialize
> the 'current_cpu' variable with the first vCPU created. The
> command executed on the monitor will end using it.
>
> This fixes:
>
> $ cat << EOF| qemu-system-i386 -M q35 -nographic -serial none -monitor stdio
> o/4 0xcf8 0x8400f841
> o/4 0xcfc 0xaa215d6d
> o/4 0x6d30 0x2ef8ffbe
> o/1 0xb2 0x20
> EOF
> Segmentation fault (core dumped)
>
> Thread 1 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
> 0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57
> 57 old_mask = cpu->interrupt_request;
> (gdb) bt
> #0 0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57
> #1 0x00005555558ed7d2 in cpu_interrupt (cpu=0x0, mask=64) at include/hw/core/cpu.h:877
> #2 0x00005555558ee776 in ich9_apm_ctrl_changed (val=32, arg=0x555556e2ff50) at hw/isa/lpc_ich9.c:442
> #3 0x0000555555b47f96 in apm_ioport_writeb (opaque=0x555556e308c0, addr=0, val=32, size=1) at hw/isa/apm.c:44
> #4 0x0000555555879931 in memory_region_write_accessor (mr=0x555556e308e0, addr=0, value=0x7fffffffb9f8, size=1, shift=0, mask=255, attrs=...) at memory.c:483
> #5 0x0000555555879b5a in access_with_adjusted_size (addr=0, value=0x7fffffffb9f8, size=4, access_size_min=1, access_size_max=1, access_fn=
> 0x55555587984e <memory_region_write_accessor>, mr=0x555556e308e0, attrs=...) at memory.c:544
> #6 0x000055555587ca32 in memory_region_dispatch_write (mr=0x555556e308e0, addr=0, data=32, op=MO_32, attrs=...) at memory.c:1465
> #7 0x000055555581b7e9 in flatview_write_continue (fv=0x55555698a790, addr=178, attrs=..., ptr=0x7fffffffbb84, len=4, addr1=0, l=4, mr=0x555556e308e0) at exec.c:3198
> #8 0x000055555581b92e in flatview_write (fv=0x55555698a790, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3238
> #9 0x000055555581bc81 in address_space_write (as=0x555556441220 <address_space_io>, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3329
> #10 0x0000555555873f08 in cpu_outl (addr=178, val=32) at ioport.c:80
> #11 0x000055555598a26a in hmp_ioport_write (mon=0x5555567621b0, qdict=0x555557702600) at monitor/misc.c:937
> #12 0x0000555555c9c5a5 in handle_hmp_command (mon=0x5555567621b0, cmdline=0x55555676aae1 "/1 0xb2 0x20") at monitor/hmp.c:1082
> #13 0x0000555555c99e02 in monitor_command_cb (opaque=0x5555567621b0, cmdline=0x55555676aae0 "o/1 0xb2 0x20", readline_opaque=0x0) at monitor/hmp.c:47
> ^
> HMP command from monitor
>
> Reported-by: Alexander Bulekov <email address hidden>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1878645
> Signed-off-by: Philippe Mathieu-Daudé <email address hidden>
> ---
> Cc: Bug 1878645 <email address hidden>
>
> RFC because I believe the correct fix is to NOT use current_cpu
> out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU.
> ---
> cpus.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/cpus.c ...

Read more...

Revision history for this message
Peter Maydell (pmaydell) wrote :

On Wed, 1 Jul 2020 at 19:21, Philippe Mathieu-Daudé <email address hidden> wrote:
>
> We can run I/O access with the 'i' or 'o' HMP commands in the
> monitor. These commands are expected to run on a vCPU. The
> monitor is not a vCPU thread. To avoid crashing, initialize
> the 'current_cpu' variable with the first vCPU created. The
> command executed on the monitor will end using it.

>
> RFC because I believe the correct fix is to NOT use current_cpu
> out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU.

Yes, I agree -- I don't think this is the correct fix.
current_cpu is documented as "only valid inside cpu_exec()",
ie if you're actually on a vcpu thread you can use it, but if
you're not on a CPU thread, like the monitor, you need to
say which vCPU you want to affect. For the monitor, that
would be the current "default cpu" as set by the "cpu"
command (cf monitor_set_cpu()). The bug here will be that
somewhere along the line we are probably missing plumbing
sufficient to pass down "which CPU do we want".

https://bugs.launchpad.net/qemu/+bug/1602247 is a bug of
a similar nature -- if the gdbstub does a memory access
we know which CPU we're trying to do that memory access as,
but it doesn't get plumbed through and so in the Arm GIC
register read/write function that looks at current_cpu
we get a segfault.

One way to fix this would be to do something akin to how
real hardware works -- encode into the MemTxAttrs an
indication of what the transaction master was (eg the
CPU number for CPUs); then the handful of devices that
care about which CPU was doing the transaction can use
that, rather than directly looking at current_cpu, and
when gdbstub or monitor perform an access that should
act like it's from a particular CPU they can indicate
that by supplying appropriate transaction attributes.
That would potentially be quite a bit of work though
(but I think it would be a neat design if we want to
try to fix this kind of "segfault if the user prods
a device via the monitor" bug).

+ if (!current_cpu) {
+ current_cpu = cpu;
+ }

Some more specific issues with this -- current_cpu is
a thread-local variable, so this will set that for
whatever thread happens to make this call, which
might or might not be the one that ends up handling
the monitor command. Also some code assumes that
current_cpu is non-NULL only if this is a vCPU thread,
eg sigbus_handler().

thanks
-- PMM

Revision history for this message
Philippe Mathieu-Daudé (philmd) wrote :
Download full text (3.2 KiB)

On 7/1/20 10:35 PM, Peter Maydell wrote:
> On Wed, 1 Jul 2020 at 19:21, Philippe Mathieu-Daudé <email address hidden> wrote:
>>
>> We can run I/O access with the 'i' or 'o' HMP commands in the
>> monitor. These commands are expected to run on a vCPU. The
>> monitor is not a vCPU thread. To avoid crashing, initialize
>> the 'current_cpu' variable with the first vCPU created. The
>> command executed on the monitor will end using it.
>
>>
>> RFC because I believe the correct fix is to NOT use current_cpu
>> out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU.
>
> Yes, I agree -- I don't think this is the correct fix.
> current_cpu is documented as "only valid inside cpu_exec()",
> ie if you're actually on a vcpu thread you can use it, but if
> you're not on a CPU thread, like the monitor, you need to
> say which vCPU you want to affect. For the monitor, that
> would be the current "default cpu" as set by the "cpu"
> command (cf monitor_set_cpu()). The bug here will be that
> somewhere along the line we are probably missing plumbing
> sufficient to pass down "which CPU do we want".

TIL mon_get_cpu() :)

This is a bit different here, the monitor is not doing an
access on a CPU address space, but directly on the I/O
address space. The APM port is registered by the ICH9
south bridge, and this triggers a SMI exception on a
CPU core, but I have no idea which one, a random one?
Then this particular core will switch to SMM mode.

Another way of seeing this problem, is imagining we
start a PIT timer from the monitor. When the timer
expires, the PIT will interrupt the CPU. Which CPU
to interrupt? We are not in the context of the monitor.

> https://bugs.launchpad.net/qemu/+bug/1602247 is a bug of
> a similar nature -- if the gdbstub does a memory access
> we know which CPU we're trying to do that memory access as,
> but it doesn't get plumbed through and so in the Arm GIC
> register read/write function that looks at current_cpu
> we get a segfault.
>
> One way to fix this would be to do something akin to how
> real hardware works -- encode into the MemTxAttrs an
> indication of what the transaction master was (eg the
> CPU number for CPUs); then the handful of devices that
> care about which CPU was doing the transaction can use
> that, rather than directly looking at current_cpu, and
> when gdbstub or monitor perform an access that should
> act like it's from a particular CPU they can indicate
> that by supplying appropriate transaction attributes.
> That would potentially be quite a bit of work though
> (but I think it would be a neat design if we want to
> try to fix this kind of "segfault if the user prods
> a device via the monitor" bug).

This is complex stuff. Too early for me to digest, I am
keeping this for later (I am not ignoring your comment).

>
> + if (!current_cpu) {
> + current_cpu = cpu;
> + }
>
> Some more specific issues with this -- current_cpu is
> a thread-local variable, so this will set that for
> whatever thread happens to make this call, which
> might or might not be the one that ends up handling
> the monitor command. Also some code assumes that
> current_cpu is non-NULL only if this is a vCPU threa...

Read more...

Revision history for this message
Alex Bennée (ajbennee) wrote :

Paolo Bonzini <email address hidden> writes:

> On 01/07/20 22:35, Peter Maydell wrote:
>> For the monitor, that
>> would be the current "default cpu" as set by the "cpu"
>> command (cf monitor_set_cpu()). The bug here will be that
>> somewhere along the line we are probably missing plumbing
>> sufficient to pass down "which CPU do we want".
>
> Yeah, the fix is probably to add a functions that returns either
> current_cpu or the monitor CPU, and use it in device emulation if
> applicable.
>
> The problem with current_cpu is that it affects stuff like run_on_cpu,
> and that is guaranteed to cause havoc to code that expects to run on a
> given CPU and therefore doesn't use locks.

IIRC the original reported bug was in a APM callback which was triggered
by an MMIO operation. Currently we don't expose the current cpu via the
memory dispatch routines. Should we to ensure there is always something
valid there?

>
> Paolo

--
Alex Bennée

Philippe Mathieu-Daudé (philmd)
Changed in qemu:
status: New → Confirmed
Revision history for this message
Alexander Bulekov (a1xndr) wrote :

I moved this report over to QEMU's new bug tracker on gitlab.com.
Please continue with the discussion here:

https://gitlab.com/qemu-project/qemu/-/issues/536

Revision history for this message
Thomas Huth (th-huth) wrote :

Thanks for moving it over! ... let's close this one here on Launchpad now.

Changed in qemu:
status: Confirmed → Invalid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.