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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.