mbox series

[v2,0/7] target/i386: Fix physical address masking bugs

Message ID 20240223130948.237186-1-pbonzini@redhat.com
Headers show
Series target/i386: Fix physical address masking bugs | expand

Message

Paolo Bonzini Feb. 23, 2024, 1:09 p.m. UTC 
The address translation logic in get_physical_address() will currently
truncate physical addresses to 32 bits unless long mode is enabled.
This is incorrect when using physical address extensions (PAE) outside
of long mode, with the result that a 32-bit operating system using PAE
to access memory above 4G will experience undefined behaviour.
Instead, truncation must be applied to the linear address.  Because
this truncation is diffent between 32- and 64-bit modes, the series
opts to split 32- and 64-bit modes to separate MMU indices, which is
the simplest way to ensure that, for example, a kernel that runs both
32-bit and 64-bit programs looks up the correct address in the
(64-bit) page tables.

Furthermore, when inspecting the code I noticed that the A20 mask is
applied incorrectly when NPT is active.  The mask should not be applied
to the addresses that are looked up in the NPT, only to the physical
addresses.  Obviously no hypervisor is going to leave A20 masking on,
but the code actually becomes simpler so let's do it.

Patches 1 and 2 fix cases in which the addresses must be masked,
or overflow is otherwise invalid, for MMU_PHYS_IDX accesses.

Patches 3 and 4 introduce separate MMU indexes for 32- and 64-bit
accesses.

Patch 5 fixes the bug, by limiting the masking to the 32-bit MMU indexes.

Patches 6 and 7 further clean up the MMU functions to centralize
application of the A20 mask and fix bugs in the process.

Tested with kvm-unit-tests SVM tests and with an old 32-bit Debian image.

Paolo Bonzini (7):
  target/i386: mask high bits of CR3 in 32-bit mode
  target/i386: check validity of VMCB addresses
  target/i386: introduce function to query MMU indices
  target/i386: use separate MMU indexes for 32-bit accesses
  target/i386: Fix physical address truncation
  target/i386: remove unnecessary/wrong application of the A20 mask
  target/i386: leave the A20 bit set in the final NPT walk

 target/i386/cpu.h                    | 46 +++++++++++++++++++-----
 target/i386/cpu.c                    |  9 +++--
 target/i386/tcg/sysemu/excp_helper.c | 52 +++++++++++++---------------
 target/i386/tcg/sysemu/misc_helper.c |  3 ++
 target/i386/tcg/sysemu/svm_helper.c  | 27 +++++++++++----
 5 files changed, 92 insertions(+), 45 deletions(-)

Comments

Michael Brown Feb. 23, 2024, 5:57 p.m. UTC | #1 
On 23/02/2024 13:09, Paolo Bonzini wrote:
> The address translation logic in get_physical_address() will currently
> truncate physical addresses to 32 bits unless long mode is enabled.
> This is incorrect when using physical address extensions (PAE) outside
> of long mode, with the result that a 32-bit operating system using PAE
> to access memory above 4G will experience undefined behaviour.
> Instead, truncation must be applied to the linear address.  Because
> this truncation is diffent between 32- and 64-bit modes, the series
> opts to split 32- and 64-bit modes to separate MMU indices, which is
> the simplest way to ensure that, for example, a kernel that runs both
> 32-bit and 64-bit programs looks up the correct address in the
> (64-bit) page tables.
> 
> Furthermore, when inspecting the code I noticed that the A20 mask is
> applied incorrectly when NPT is active.  The mask should not be applied
> to the addresses that are looked up in the NPT, only to the physical
> addresses.  Obviously no hypervisor is going to leave A20 masking on,
> but the code actually becomes simpler so let's do it.
> 
> Patches 1 and 2 fix cases in which the addresses must be masked,
> or overflow is otherwise invalid, for MMU_PHYS_IDX accesses.
> 
> Patches 3 and 4 introduce separate MMU indexes for 32- and 64-bit
> accesses.
> 
> Patch 5 fixes the bug, by limiting the masking to the 32-bit MMU indexes.
> 
> Patches 6 and 7 further clean up the MMU functions to centralize
> application of the A20 mask and fix bugs in the process.
> 
> Tested with kvm-unit-tests SVM tests and with an old 32-bit Debian image.
> 
> Paolo Bonzini (7):
>    target/i386: mask high bits of CR3 in 32-bit mode
>    target/i386: check validity of VMCB addresses
>    target/i386: introduce function to query MMU indices
>    target/i386: use separate MMU indexes for 32-bit accesses
>    target/i386: Fix physical address truncation
>    target/i386: remove unnecessary/wrong application of the A20 mask
>    target/i386: leave the A20 bit set in the final NPT walk
> 
>   target/i386/cpu.h                    | 46 +++++++++++++++++++-----
>   target/i386/cpu.c                    |  9 +++--
>   target/i386/tcg/sysemu/excp_helper.c | 52 +++++++++++++---------------
>   target/i386/tcg/sysemu/misc_helper.c |  3 ++
>   target/i386/tcg/sysemu/svm_helper.c  | 27 +++++++++++----
>   5 files changed, 92 insertions(+), 45 deletions(-)

Thank you for putting in the work to fix this all up!

I confirm that this patch series resolves the issue that I originally 
observed in https://gitlab.com/qemu-project/qemu/-/issues/2040 and that 
Windows 10 32-bit is able to boot with PAE enabled and memory over 4G.

Tested-by: Michael Brown <mcb30@ipxe.org>

Thanks,

Michael