Re: [PATCH for-8.2?] target/i386: Fix 32-bit wrapping of pc/eip computation

[Richard Henderson <richard.henderson@linaro.org>]

On 12/12/23 13:08, Paolo Bonzini wrote:
> On Tue, Dec 12, 2023 at 6:25 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> In 32-bit mode, pc = eip + cs_base is also 32-bit, and must wrap.
>> Failure to do so results in incorrect memory exceptions to the guest.
>> Before 732d548732ed, this was implicitly done via truncation to
>> target_ulong but only in qemu-system-i386, not qemu-system-x86_64.
>>
>> To fix this, we must add conditional zero-extensions.
>> Since we have to test for 32 vs 64-bit anyway, note that cs_base
>> is always zero in 64-bit mode.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2022
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>
>> This may be too late for 8.2; if not, then 8.2.1 and 8.1.next.
>> I think I have found all forms of pc <-> eip, but another set
>> of eyes would be appreciated.
> 
> Looks good, but perhaps you could also squash the following?
> 
> diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
> index 2c6a12c8350..83ee89579b8 100644
> --- a/target/i386/tcg/tcg-cpu.c
> +++ b/target/i386/tcg/tcg-cpu.c
> @@ -52,7 +52,11 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs,
>       /* The instruction pointer is always up to date with CF_PCREL. */
>       if (!(tb_cflags(tb) & CF_PCREL)) {
>           CPUX86State *env = cpu_env(cs);
> -        env->eip = tb->pc - tb->cs_base;
> +        if (tb->flags & HF_CS64_MASK) {
> +            env->eip = tb->pc;
> +        } else {
> +            env->eip = (uint32_t) (tb->pc - tb->cs_base);
> +        }
>       }
>   }
> 
> 
> It wouldn't be the same bug as 2022 (it wouldn't be new with the vaddr
> change) so it's okay to sort out this extra case after release.

Good catch, I'll squash it.  Thanks.


r~