IO port write width clamping differs between TCG and KVM

IO port write width clamping differs between TCG and KVM

[Laszlo Ersek]

(resending with qemu-devel on CC; sorry!)

Hi,

this is with QEMU-7.2.

"docs/specs/acpi_cpu_hotplug.rst" writes:

> Legacy ACPI CPU hotplug interface registers
> -------------------------------------------
> ...
> - The first DWORD in bitmap is used in write mode to switch from legacy
>   to modern CPU hotplug interface, write 0 into it to do switch.

and

> (x86) Detecting and enabling modern CPU hotplug interface
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> QEMU starts with legacy CPU hotplug interface enabled. Detecting and
> switching to modern interface is based on the 2 legacy CPU hotplug features:
>
> #. Writes into CPU bitmap are ignored.
> #. CPU bitmap always has bit #0 set, corresponding to boot CPU.
>
> Use following steps to detect and enable modern CPU hotplug interface:
>
> #. Store 0x0 to the 'CPU selector' register, attempting to switch to modern mode
> #. Store 0x0 to the 'CPU selector' register, to ensure valid selector value
> #. Store 0x0 to the 'Command field' register
> #. Read the 'Command data 2' register.
>    If read value is 0x0, the modern interface is enabled.
>    Otherwise legacy or no CPU hotplug interface available

OVMF does exactly that.

The detection method works on KVM, and does not work on TCG.

I've managed to track it as far as follows:

In "hw/acpi/cpu_hotplug.c", we have:

> static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
> {
>     AcpiCpuHotplug *cpus = opaque;
>     uint64_t val = cpus->sts[addr];
>
>     return val;
> }
>
> static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
>                              unsigned int size)
> {
>     /* firmware never used to write in CPU present bitmap so use
>        this fact as means to switch QEMU into modern CPU hotplug
>        mode by writing 0 at the beginning of legacy CPU bitmap
>      */
>     if (addr == 0 && data == 0) {
>         AcpiCpuHotplug *cpus = opaque;
>         object_property_set_bool(cpus->device, "cpu-hotplug-legacy", false,
>                                  &error_abort);
>     }
> }
>
> static const MemoryRegionOps AcpiCpuHotplug_ops = {
>     .read = cpu_status_read,
>     .write = cpu_status_write,
>     .endianness = DEVICE_LITTLE_ENDIAN,
>     .valid = {
>         .min_access_size = 1,
>         .max_access_size = 1,
>     },
> };

The cpu_status_write() function is reached with KVM acceleration, when
OVMF writes to the *DWORD* IO Port register at offset 0 (CPU selector).
The function is entered with the following parameters:

  addr=0, data=0, size=1

This means that something in the address space machinery *splits* the
DWORD access from OVMF, into single byte accesses. Consequently, the
behavior is as expected -- the object_property_set_bool() call is
reached, and the register block is switched over to "modern" mode.
Everything's fine.

The full backtrace at this moment is:

> #0  cpu_status_write (opaque=0x5555572b9880, addr=0, data=0, size=1) at ../../src/upstream/qemu/hw/acpi/cpu_hotplug.c:42
> #1  0x0000555555d3d34f in memory_region_write_accessor (mr=0x5555572b9890, addr=0, value=0x7fffe6412fe8, size=1, shift=0, mask=255, attrs=...) at ../../src/upstream/qemu/softmmu/memory.c:493
>         tmp = 0
> #2  0x0000555555d3d595 in access_with_adjusted_size (addr=0, value=0x7fffe6412fe8, size=1, access_size_min=1, access_size_max=4, access_fn=0x555555d3d259 <memory_region_write_accessor>, mr=0x5555572b9890, attrs=...)
>     at ../../src/upstream/qemu/softmmu/memory.c:555
>         access_mask = 255
>         access_size = 1
>         i = 0
>         r = 0
> #3  0x0000555555d406d8 in memory_region_dispatch_write (mr=0x5555572b9890, addr=0, data=0, op=MO_8, attrs=...) at ../../src/upstream/qemu/softmmu/memory.c:1515
>         size = 1
> #4  0x0000555555d4e320 in flatview_write_continue (fv=0x7fffdc0141e0, addr=3288, attrs=..., ptr=0x7ffff7fbd000, len=4, addr1=0, l=1, mr=0x5555572b9890) at ../../src/upstream/qemu/softmmu/physmem.c:2825
>         ram_ptr = 0x4 <error: Cannot access memory at address 0x4>
>         val = 0
>         result = 0
>         release_lock = true
>         buf = 0x7ffff7fbd000 ""
> #5  0x0000555555d4e483 in flatview_write (fv=0x7fffdc0141e0, addr=3288, attrs=..., buf=0x7ffff7fbd000, len=4) at ../../src/upstream/qemu/softmmu/physmem.c:2867
>         l = 4
>         addr1 = 0
>         mr = 0x5555572b9890
> #6  0x0000555555d4e833 in address_space_write (as=0x55555690d000 <address_space_io>, addr=3288, attrs=..., buf=0x7ffff7fbd000, len=4) at ../../src/upstream/qemu/softmmu/physmem.c:2963
>         _rcu_read_auto = 0x1
>         result = 0
>         fv = 0x7fffdc0141e0
> #7  0x0000555555d4e8a0 in address_space_rw (as=0x55555690d000 <address_space_io>, addr=3288, attrs=..., buf=0x7ffff7fbd000, len=4, is_write=true) at ../../src/upstream/qemu/softmmu/physmem.c:2973
> #8  0x0000555555de1c6e in kvm_handle_io (port=3288, attrs=..., data=0x7ffff7fbd000, direction=1, size=4, count=1) at ../../src/upstream/qemu/accel/kvm/kvm-all.c:2639
>         i = 0
>         ptr = 0x7ffff7fbd000 ""
> #9  0x0000555555de240e in kvm_cpu_exec (cpu=0x555556d42db0) at ../../src/upstream/qemu/accel/kvm/kvm-all.c:2890
>         attrs = {unspecified = 0, secure = 0, user = 0, memory = 0, requester_id = 0, byte_swap = 0, target_tlb_bit0 = 0, target_tlb_bit1 = 0, target_tlb_bit2 = 0}
>         run = 0x7ffff7fbc000
>         ret = 0
>         run_ret = 0
> #10 0x0000555555de51a2 in kvm_vcpu_thread_fn (arg=0x555556d42db0) at ../../src/upstream/qemu/accel/kvm/kvm-accel-ops.c:51
>         cpu = 0x555556d42db0
>         r = 65536
> #11 0x0000555555feb23a in qemu_thread_start (args=0x555556d46240) at ../../src/upstream/qemu/util/qemu-thread-posix.c:505
>         __cancel_buf =
>             {__cancel_jmp_buf = {{__cancel_jmp_buf = {140737056437824, -6609603676087882481, 140737056437824, 7, 140737304061232, 0, -6609603676064813809, -1076185459444912881}, __mask_was_saved = 0}}, __pad = {0x7fffe6413330, 0x0, 0x0, 0x0}}
>         __cancel_routine = 0x555555feb0e9 <qemu_thread_atexit_notify>
>         __cancel_arg = 0x0
>         __not_first_call = 0
>         qemu_thread_args = 0x555556d46240
>         start_routine = 0x555555de50d6 <kvm_vcpu_thread_fn>
>         arg = 0x555556d42db0
>         r = 0x0
> #12 0x00007ffff503e802 in start_thread () at /lib64/libc.so.6
> #13 0x00007ffff4fde450 in clone3 () at /lib64/libc.so.6

Further invocations of the same function happen with:

  addr=1, data=0, size=1
  addr=2, data=0, size=1
  addr=3, data=0, size=1

which are (correctly) ignored, per the (addr == 0 && data == 0)
condition inside the function.

The function is not called ever again -- that's because further accesses
to the register block happen through the "modern" memory region, set up
through the following call tree, initiated by cpu_status_write():

  ich9_pm_set_cpu_hotplug_legacy()       [hw/acpi/ich9.c]
    acpi_switch_to_modern_cphp()         [hw/acpi/cpu_hotplug.c]
      memory_region_del_subregion(...)
      cpu_hotplug_hw_init()              [hw/acpi/cpu.c]
        memory_region_init_io(...)
        memory_region_add_subregion(...)

In other words, when cpu_status_write() is reached "by design", the
original memory region that handled the trap is replaced with a new one,
and afterwards, the write handler is a different function.

So, all of this is fine.


When using TCG acceleration however, the cpu_status_write() function is
not reached by the first DWORD write (which OVMF performs according to
the spec quoted above). Instead, the first entry looks like this:

  addr=5, data=0, size=1

This corresponds to the *third* step OVMF performs:

> #. Store 0x0 to the 'Command field' register

but this write should not reach the *legacy* interface, only the
*modern* interface (on QEMU-7.2).

In other words, the first DWORD write is prevented by the memory
machinery from reaching the write handler, but *only* with TCG. And so
the switch-over never happens.

Now, of course there is another reason for the switch-over not to occur,
and that is: the QEMU version executing OVMF simply does not support the
modern interface. That's what the negotiation steps quoted above are
supposed to find out!

Namely, the last step of the spec above is:

> #. Read the 'Command data 2' register.
>    If read value is 0x0, the modern interface is enabled.
>    Otherwise legacy or no CPU hotplug interface available

However: the "command data 2" register is *also* a DWORD register!

And, with TCG, it does read as zero, but not because the modern
interface was enabled. Instead, it reads *misleadingly* as zero because
the cpu_status_read() accessor is never reached *either* (with TCG).
Something in the memory view machinery stops the access from reaching
the handler, and fakes a zero retval.

If it were only the switch-over that didn't work with TCG, but OVMF
could determine that it didn't happen, that would be fine -- we'd just
say that on TCG, you don't get CPU hot(un)plug with OVMF. But the
misbehavior on the read access *too* tricks OVMF into making the
opposite conclusion -- it assumes that the modern interface is enabled.

The rest of the CPU counting is then totally busted, as OVMF thinks it
talks to the modern interface, and "determines" that the system has 1
possible CPU, of which *zero* are present.

Now, if I look at the above-quoted backtrace, I see that, from frame#5
to frame#4, a kind of "streaming" happens (with KVM only). Because,
len=4 is there, but "l=1" appears upon entry to
flatview_write_continue():

  flatview_write()                         [softmmu/physmem.c]
    flatview_translate()                   [softmmu/physmem.c]
      flatview_do_translate()              [softmmu/physmem.c]
        address_space_translate_internal() [softmmu/physmem.c]
    flatview_write_continue()              [softmmu/physmem.c]

And then I vaguely feel that this is somehow related to the following
big comment in address_space_translate_internal() [softmmu/physmem.c]:

>     /* MMIO registers can be expected to perform full-width accesses based only
>      * on their address, without considering adjacent registers that could
>      * decode to completely different MemoryRegions.  When such registers
>      * exist (e.g. I/O ports 0xcf8 and 0xcf9 on most PC chipsets), MMIO
>      * regions overlap wildly.  For this reason we cannot clamp the accesses
>      * here.
>      *
>      * If the length is small (as is the case for address_space_ldl/stl),
>      * everything works fine.  If the incoming length is large, however,
>      * the caller really has to do the clamping through memory_access_size.
>      */

What I don't understand is the *difference* in behavior between KVM and
TCG. The above reasoning either applies to both KVM and TCG, or it
applies to neither, right?

What needs fixing here? TCG, or the CPU hotplug code?

Thanks,
Laszlo

Re: IO port write width clamping differs between TCG and KVM

[Laszlo Ersek]

On 1/3/23 18:42, Laszlo Ersek wrote:
> (resending with qemu-devel on CC; sorry!)
> 
> Hi,
> 
> this is with QEMU-7.2.

This is a regression. It works fine with QEMU-5.0. The regression has
not been fixed since QEMU-7.2, as of master @ 222059a0fccf ("Merge tag
'pull-ppc-20221221' of https://gitlab.com/danielhb/qemu into staging",
2022-12-21).

I'm bisecting.

(It's a relief that this is a regression. I felt pretty certain that I
had tested CPU hotplug with TCG as well!

I've picked QEMU-5.0 as the start candidate for my bisection for the
following reason: per git-blame, Igor described the modern interface
detection steps in commit ae340aa3d2567 (which I reviewed), and the
first tag/release to contain that commit was QEMU-5.0. The first QEMU
release after Igor and I had worked on this in QEMU and OVMF definitely
worked with TCG too, by my account.)

Laszlo

Re: IO port write width clamping differs between TCG and KVM

[Laszlo Ersek]

On 1/4/23 07:06, Laszlo Ersek wrote:
> On 1/3/23 18:42, Laszlo Ersek wrote:
>> (resending with qemu-devel on CC; sorry!)
>>
>> Hi,
>>
>> this is with QEMU-7.2.
> 
> This is a regression. It works fine with QEMU-5.0. The regression has
> not been fixed since QEMU-7.2, as of master @ 222059a0fccf ("Merge tag
> 'pull-ppc-20221221' of https://gitlab.com/danielhb/qemu into staging",
> 2022-12-21).
> 
> I'm bisecting.

Something's definitely going on with TCG. At v6.0.0-1974-ge0da9171e02f,
I cannot even *start* the domain I'm using for testing, because QEMU
errors out with:

qemu-system-i386: -accel tcg: failed to map shared memory for execute:
Permission denied

Laszlo

Re: IO port write width clamping differs between TCG and KVM

[Laszlo Ersek]

[-- Attachment #1: Type: text/plain, Size: 2904 bytes --]

Adding Michael. The thread started here:

https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg00199.html

More below:

On 1/4/23 07:06, Laszlo Ersek wrote:
> On 1/3/23 18:42, Laszlo Ersek wrote:
>> (resending with qemu-devel on CC; sorry!)
>>
>> Hi,
>>
>> this is with QEMU-7.2.
>
> This is a regression. It works fine with QEMU-5.0. The regression has
> not been fixed since QEMU-7.2, as of master @ 222059a0fccf ("Merge tag
> 'pull-ppc-20221221' of https://gitlab.com/danielhb/qemu into staging",
> 2022-12-21).
>
> I'm bisecting.
>
> (It's a relief that this is a regression. I felt pretty certain that I
> had tested CPU hotplug with TCG as well!
>
> I've picked QEMU-5.0 as the start candidate for my bisection for the
> following reason: per git-blame, Igor described the modern interface
> detection steps in commit ae340aa3d2567 (which I reviewed), and the
> first tag/release to contain that commit was QEMU-5.0. The first QEMU
> release after Igor and I had worked on this in QEMU and OVMF
> definitely worked with TCG too, by my account.)

See the bisection log attached.

The first bad commit is:

> commit 5d971f9e672507210e77d020d89e0e89165c8fc9
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Wed Jun 10 09:47:49 2020 -0400
>
>     memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"
>
>     Memory API documentation documents valid .min_access_size and .max_access_size
>     fields and explains that any access outside these boundaries is blocked.
>
>     This is what devices seem to assume.
>
>     However this is not what the implementation does: it simply
>     ignores the boundaries unless there's an "accepts" callback.
>
>     Naturally, this breaks a bunch of devices.
>
>     Revert to the documented behaviour.
>
>     Devices that want to allow any access can just drop the valid field,
>     or add the impl field to have accesses converted to appropriate
>     length.
>
>     Cc: qemu-stable@nongnu.org
>     Reviewed-by: Richard Henderson <rth@twiddle.net>
>     Fixes: CVE-2020-13754
>     Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1842363
>     Fixes: a014ed07bd5a ("memory: accept mismatching sizes in memory_region_access_valid")
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>     Message-Id: <20200610134731.1514409-1-mst@redhat.com>
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

First released in QEMU v5.1.0 and v6.0.0 (exact same commit hash in
both, as v6.0.0 descends of v5.1.0).

Because this is a CVE fix, and also because of the suggestions made in
the commit message, I think the commit is actually right, and what we
need to fix is the hotplug register block -- namely the
"AcpiCpuHotplug_ops" structure in "hw/acpi/cpu_hotplug.c".

However, what I *really* don't understand is why this commit
(5d971f9e672507210e77d020d89e0e89165c8fc9) makes a difference *only*
under TCG.

Laszlo

[-- Attachment #2: bisect.log --]
[-- Type: text/x-log, Size: 2862 bytes --]

git bisect start
# bad: [222059a0fccf4af3be776fe35a5ea2d6a68f9a0b] Merge tag 'pull-ppc-20221221' of https://gitlab.com/danielhb/qemu into staging
git bisect bad 222059a0fccf4af3be776fe35a5ea2d6a68f9a0b
# good: [fdd76fecdde1ad444ff4deb7f1c4f7e4a1ef97d6] Update version for v5.0.0 release
git bisect good fdd76fecdde1ad444ff4deb7f1c4f7e4a1ef97d6
# bad: [609d7596524ab204ccd71ef42c9eee4c7c338ea4] Update version for v6.0.0 release
git bisect bad 609d7596524ab204ccd71ef42c9eee4c7c338ea4
# bad: [a0bdf866873467271eff9a92f179ab0f77d735cb] Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20201012a' into staging
git bisect bad a0bdf866873467271eff9a92f179ab0f77d735cb
# bad: [3a9163af4e3dd61795a35d47b702e302f98f81d6] Merge remote-tracking branch 'remotes/philmd-gitlab/tags/sdcard-CVE-2020-13253-pull-request' into staging
git bisect bad 3a9163af4e3dd61795a35d47b702e302f98f81d6
# good: [7d3660e79830a069f1848bb4fa1cdf8f666424fb] Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging
git bisect good 7d3660e79830a069f1848bb4fa1cdf8f666424fb
# bad: [92fbc3e07eb924bd5e03d22764e637b2e02e46bd] vhost_net: use the function qemu_get_peer
git bisect bad 92fbc3e07eb924bd5e03d22764e637b2e02e46bd
# good: [495134b75cca3e6a34d4233113c5143439061771] hw/riscv: sifive: Change SiFive E/U CPU reset vector to 0x1004
git bisect good 495134b75cca3e6a34d4233113c5143439061771
# good: [4e5df0369fb5a49ed26518dccffeb03a252352db] adb: add autopoll_blocked variable to block autopoll
git bisect good 4e5df0369fb5a49ed26518dccffeb03a252352db
# good: [c7459633baa71d1781fde4a245d6ec9ce2f008cf] target/arm: Enable MTE
git bisect good c7459633baa71d1781fde4a245d6ec9ce2f008cf
# bad: [3591ddd39987cbdaa0cfa344a262f315abd97582] Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging
git bisect bad 3591ddd39987cbdaa0cfa344a262f315abd97582
# bad: [1f18a1e6ab8368a4eab2d22894d3b2ae75250cd3] target/i386: reimplement fyl2x using floatx80 operations
git bisect bad 1f18a1e6ab8368a4eab2d22894d3b2ae75250cd3
# bad: [ee760ac80ac1f130138e8eb4eba263a4d48ace51] hw/scsi/megasas: Fix possible out-of-bounds array access in tracepoints
git bisect bad ee760ac80ac1f130138e8eb4eba263a4d48ace51
# bad: [5d971f9e672507210e77d020d89e0e89165c8fc9] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"
git bisect bad 5d971f9e672507210e77d020d89e0e89165c8fc9
# good: [ae2b72072bc401114ebb9f46abd67d07d4cabf10] util/getauxval: Porting to FreeBSD getauxval feature
git bisect good ae2b72072bc401114ebb9f46abd67d07d4cabf10
# good: [4b7c06837ae0b1ff56473202a42e7e386f53d6db] libqos: pci-pc: use 32-bit write for EJ register
git bisect good 4b7c06837ae0b1ff56473202a42e7e386f53d6db
# first bad commit: [5d971f9e672507210e77d020d89e0e89165c8fc9] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

Re: IO port write width clamping differs between TCG and KVM

[Philippe Mathieu-Daudé]

Hi Laszlo,

Happy new year!

On 4/1/23 08:04, Laszlo Ersek wrote:
> Adding Michael. The thread started here:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg00199.html
> 
> More below:
> 
> On 1/4/23 07:06, Laszlo Ersek wrote:
>> On 1/3/23 18:42, Laszlo Ersek wrote:
>>> (resending with qemu-devel on CC; sorry!)
>>>
>>> Hi,
>>>
>>> this is with QEMU-7.2.
>>
>> This is a regression. It works fine with QEMU-5.0. The regression has
>> not been fixed since QEMU-7.2, as of master @ 222059a0fccf ("Merge tag
>> 'pull-ppc-20221221' of https://gitlab.com/danielhb/qemu into staging",
>> 2022-12-21).
>>
>> I'm bisecting.
>>
>> (It's a relief that this is a regression. I felt pretty certain that I
>> had tested CPU hotplug with TCG as well!
>>
>> I've picked QEMU-5.0 as the start candidate for my bisection for the
>> following reason: per git-blame, Igor described the modern interface
>> detection steps in commit ae340aa3d2567 (which I reviewed), and the
>> first tag/release to contain that commit was QEMU-5.0. The first QEMU
>> release after Igor and I had worked on this in QEMU and OVMF
>> definitely worked with TCG too, by my account.)
> 
> See the bisection log attached.
> 
> The first bad commit is:
> 
>> commit 5d971f9e672507210e77d020d89e0e89165c8fc9
>> Author: Michael S. Tsirkin <mst@redhat.com>
>> Date:   Wed Jun 10 09:47:49 2020 -0400
>>
>>      memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

Good, I was going to point at this commit but you were faster :) This
commit has been problematic (because not all code base was ready):

$ git log 5d971f9e67.. | fgrep 5d971f9e67
     This was not an issue until commit 5d971f9e67 which reverted
     This is particularly useful since commit 5d971f9e67 which reverted
     This was not an issue until commit 5d971f9e67 which reverted
     was allowed before commit 5d971f9e67 ("memory: Revert "memory: accept
     Fixes: 5d971f9e67 ("memory: Revert "memory: accept mismatching 
sizes in memory_region_access_valid"")
     Commit 5d971f9e6725 ("memory: Revert "memory: accept mismatching sizes
     Fixes: 5d971f9e6725 ("memory: Revert "memory: accept mismatching 
sizes in memory_region_access_valid")
     Fixes: 5d971f9e67 (memory: Revert "memory: accept mismatching sizes 
in memory_region_access_valid")
     5d971f9e6725 ("memory: Revert "memory: accept mismatching sizes in 
memory_region_access_valid"")
     Commit 5d971f9e672507210e77d020d89e0e89165c8fc9

See the later commit dba04c3488:

commit dba04c3488c4699f5afe96f66e448b1d447cf3fb
Author: Michael Tokarev <mjt@tls.msk.ru>
Date:   Mon Jul 20 19:06:27 2020 +0300

     acpi: accept byte and word access to core ACPI registers

     All ISA registers should be accessible as bytes, words or dwords
     (if wide enough).  Fix the access constraints for acpi-pm-evt,
     acpi-pm-tmr & acpi-cnt registers.

     Fixes: 5d971f9e67 (memory: Revert "memory: accept mismatching sizes 
in memory_region_access_valid")
     Fixes: afafe4bbe0 (apci: switch cnt to memory api)
     Fixes: 77d58b1e47 (apci: switch timer to memory api)
     Fixes: b5a7c024d2 (apci: switch evt to memory api)
     Buglink: 
https://lore.kernel.org/xen-devel/20200630170913.123646-1-anthony.perard@citrix.com/T/
     Buglink: https://bugs.debian.org/964793
     BugLink: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=964247
     BugLink: https://bugs.launchpad.net/bugs/1886318
     Reported-By: Simon John <git@the-jedi.co.uk>
     Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
     Message-Id: <20200720160627.15491-1-mjt@msgid.tls.msk.ru>
     Cc: qemu-stable@nongnu.org
     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

>>      Memory API documentation documents valid .min_access_size and .max_access_size
>>      fields and explains that any access outside these boundaries is blocked.
>>
>>      This is what devices seem to assume.
>>
>>      However this is not what the implementation does: it simply
>>      ignores the boundaries unless there's an "accepts" callback.
>>
>>      Naturally, this breaks a bunch of devices.
>>
>>      Revert to the documented behaviour.
>>
>>      Devices that want to allow any access can just drop the valid field,
>>      or add the impl field to have accesses converted to appropriate
>>      length.
>>
>>      Cc: qemu-stable@nongnu.org
>>      Reviewed-by: Richard Henderson <rth@twiddle.net>
>>      Fixes: CVE-2020-13754
>>      Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1842363
>>      Fixes: a014ed07bd5a ("memory: accept mismatching sizes in memory_region_access_valid")
>>      Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>      Message-Id: <20200610134731.1514409-1-mst@redhat.com>
>>      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> First released in QEMU v5.1.0 and v6.0.0 (exact same commit hash in
> both, as v6.0.0 descends of v5.1.0).
> 
> Because this is a CVE fix, and also because of the suggestions made in
> the commit message, I think the commit is actually right, and what we
> need to fix is the hotplug register block -- namely the
> "AcpiCpuHotplug_ops" structure in "hw/acpi/cpu_hotplug.c".

Do you see anything running with '-d guest_errors'? (on top of commit
21786c7e59 "softmmu/memory: Log invalid memory accesses", or cherry-pick 
it if you are around dba04c3488 "acpi: accept byte and word access to 
core ACPI registers").

> However, what I *really* don't understand is why this commit
> (5d971f9e672507210e77d020d89e0e89165c8fc9) makes a difference *only*
> under TCG.

Is it easily reproducible with TCG?

Regards,

Phil.

Re: IO port write width clamping differs between TCG and KVM

[Laszlo Ersek]

(Adding Gerd.)

On 1/4/23 08:23, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
>
> Happy new year!

Happy new year! :) Sorry for not responding earlier, I was busy writing
& testing the patch.

[...]

>> However, what I *really* don't understand is why this commit
>> (5d971f9e672507210e77d020d89e0e89165c8fc9) makes a difference *only*
>> under TCG.

So this remains a mistery to me...

>
> Is it easily reproducible with TCG?

Yes and no.

If you have an OVMF binary built for the DEBUG or NOOPT target, then
it's nearly trivial:

qemu-system-i386 \
  -M accel=tcg \
  -smp 2 \
  -drive if=pflash,file=edk2-i386-code.fd,readonly=on \
  -drive if=pflash,file=edk2-i386-vars.fd,snapshot=on \
  -m 512 \
  -global isa-debugcon.iobase=0x402 \
  -debugcon file:/tmp/debug.log

Without the fix, you find near the top of "/tmp/debug.log":

  MaxCpuCountInitialization: QEMU v2.7 reset bug: BootCpuCount=2 Present=0
  MaxCpuCountInitialization: BootCpuCount=0 mMaxCpuCount=1

Note that here OVMF is led to believe that the system has 2 possible
CPUs, and *zero* enabled one :)

(Side comment: don't be confused by the "QEMU v2.7 reset bug" message.
That is a workaround in OVMF for a *different* QEMU bug. Unfortunately,
the workaround is unexpectedly triggered by *this* other QEMU bug as
well, but *opposite* to the intent.)

With the fix, you see, in the log:

  MaxCpuCountInitialization: BootCpuCount=2 mMaxCpuCount=2

Unfortunately, this simple reproducer does not work with the
QEMU-bundled binaries, because of commit ca26041500eb ("edk2: switch to
release builds", 2022-03-15). The switch to the RELEASE target indeed
speeds up the OVMF binaries, but it also eliminates all ASSERT()s and
DEBUG messages from the code. Therefore you'll get an empty
"/tmp/debug.log" with the above command.

Gerd, any particular reason for commit ca26041500eb? (The commit doesn't
say.)

I've found another small omission, namely in the same  patch series that
contains the above commit. While commit 3891a5996fee ("edk2: update
binaries to stable202202", 2022-03-15) updated the binaries, there was
no separate patch to refresh the accompanying text in "pc-bios/README".

For an earlier example, refer to 419236601eb2 ("pc-bios: update the
README file with edk2-stable202008 information", 2020-09-13).

Thus, at the moment, "pc-bios/README" is stale.

I'd recommend two things:

- restoring (or re-introducing, as separate binaries?) the DEBUG builds,

- refreshing "pc-bios/README".

Laszlo