Re: [PATCH 3/5] target/i386: Fix physical address truncation

[Michael Brown]

On 22/12/2023 17:59, 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.
>
> The truncation code was originally introduced in commit 33dfdb5 ("x86:
> only allow real mode to access 32bit without LMA"), where it applied
> only to translations performed while paging is disabled (and so cannot
> affect guests using PAE).
>
> Commit 9828198 ("target/i386: Add MMU_PHYS_IDX and MMU_NESTED_IDX")
> rearranged the code such that the truncation also applied to the use
> of MMU_PHYS_IDX and MMU_NESTED_IDX.  Commit 4a1e9d4 ("target/i386: Use
> atomic operations for pte updates") brought this truncation into scope
> for page table entry accesses, and is the first commit for which a
> Windows 10 32-bit guest will reliably fail to boot if memory above 4G
> is present.
>
> The truncation code however is not completely redundant.  Even though the
> maximum address size for any executed instruction is 32 bits, helpers for
> operations such as BOUND, FSAVE or XSAVE may ask get_physical_address()
> to translate an address outside of the 32-bit range, if invoked with an
> argument that is close to the 4G boundary.

Thank you for investigating and updating this patch.

I am confused by how BOUND can result in an access to a linear address 
outside of the address-size range.  I don't know the internals well 
enough, but I'm guessing it might be in the line in helper_boundl():

    high = cpu_ldl_data_ra(env, a0 + 4, GETPC());

where an address is calculated as (a0+4) using a 64-bit target_ulong 
type with no truncation to 32 bits applied.

If so, then ought the truncation to be applied on this line instead (and 
the equivalent in helper_boundw())?  My understanding (which may well be 
incorrect) is that the linear address gets truncated to the instruction 
address size (16 or 32 bits) before any conversion to a physical address 
takes place.

Regardless: this updated patch (in isolation) definitely fixes the issue 
that I observed, so I'm happy for an added

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

Thanks,

Michael