Re: [PATCH] target/i386: Fix physical address truncation when PAE is enabled

[Michael Brown]

On 20/12/2023 04:22, Richard Henderson wrote:
> On 12/18/23 23:56, Michael Brown 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.
>>
>> <snip>
> > --- a/target/i386/tcg/sysemu/excp_helper.c
> > +++ b/target/i386/tcg/sysemu/excp_helper.c
> > @@ -582,12 +582,10 @@ static bool get_physical_address(CPUX86State 
> > *env, vaddr addr,
> >       /* Translation disabled. */
> >       out->paddr = addr & x86_get_a20_mask(env);
> > -#ifdef TARGET_X86_64
> > -    if (!(env->hflags & HF_LMA_MASK)) {
> > -        /* Without long mode we can only address 32bits in real mode */
> > +    if (!(env->cr[4] & CR4_PAE_MASK)) {
> > +        /* Without PAE we can address only 32 bits */
> >           out->paddr = (uint32_t)out->paddr;
> >       }
> > -#endif
>
> This is not the correct refactoring.
>
> I agree that what we're currently doing is wrong, esp for MMU_PHYS_IDX, 
> but for the default case, if CR0.PG == 0, then CR4.PAE is ignored (vol 
> 3, section 4.1.1).
>
> I suspect the correct fix is to have MMU_PHYS_IDX pass through the input 
> address unchanged, and it is the responsibility of the higher level 
> paging mmu_idx to truncate physical addresses per PG_MODE_* before 
> recursing.

Thank you for reviewing, and for confirming the bug.

For the MMU_PHYS_IDX case, I agree that it makes sense for the address 
to be passed through unchanged.

For the default case, I think it would make sense to unconditionally 
truncate the address to 32 bits if paging is disabled.  (I am not sure 
why the original commit 33dfdb5 included a test for long mode, since I 
do not see how it is possible to get the CPU into long mode with paging 
disabled.)

I do not know what ought to be done in the MMU_NESTED_IDX case, and 
would appreciate your input on this.

Thanks,

Michael