[PATCH for-9.1] target/i386: Fix tss access size in switch_tss_ra

[PATCH for-9.1] target/i386: Fix tss access size in switch_tss_ra

[Richard Henderson]

The two limit_max variables represent size - 1, just like the
encoding in the GDT, thus the 'old' access was off by one.
Access the minimal size of the new tss: the complete tss contains
the iopb, which may be a larger block than the access api expects,
and irrelevant because the iopb is not accessed during the
switch itself.

Fixes: 8b131065080a ("target/i386/tcg: use X86Access for TSS access")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2511
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/seg_helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c
index bab552cd53..3b8fd827e1 100644
--- a/target/i386/tcg/seg_helper.c
+++ b/target/i386/tcg/seg_helper.c
@@ -378,7 +378,7 @@ static int switch_tss_ra(CPUX86State *env, int tss_selector,
 
     /* X86Access avoids memory exceptions during the task switch */
     mmu_index = cpu_mmu_index_kernel(env);
-    access_prepare_mmu(&old, env, env->tr.base, old_tss_limit_max,
+    access_prepare_mmu(&old, env, env->tr.base, old_tss_limit_max + 1,
                        MMU_DATA_STORE, mmu_index, retaddr);
 
     if (source == SWITCH_TSS_CALL) {
@@ -386,7 +386,8 @@ static int switch_tss_ra(CPUX86State *env, int tss_selector,
         probe_access(env, tss_base, 2, MMU_DATA_STORE,
                      mmu_index, retaddr);
     }
-    access_prepare_mmu(&new, env, tss_base, tss_limit,
+    /* While true tss_limit may be larger, we don't access the iopb here. */
+    access_prepare_mmu(&new, env, tss_base, tss_limit_max + 1,
                        MMU_DATA_LOAD, mmu_index, retaddr);
 
     /* save the current state in the old TSS */
-- 
2.43.0

Re: [PATCH for-9.1] target/i386: Fix tss access size in switch_tss_ra

[Peter Maydell]

On Mon, 19 Aug 2024 at 08:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The two limit_max variables represent size - 1, just like the
> encoding in the GDT, thus the 'old' access was off by one.
> Access the minimal size of the new tss: the complete tss contains
> the iopb, which may be a larger block than the access api expects,
> and irrelevant because the iopb is not accessed during the
> switch itself.
>
> Fixes: 8b131065080a ("target/i386/tcg: use X86Access for TSS access")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2511
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/i386/tcg/seg_helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Not an x86 expert but this looks OK based on
description and what we were doing before 8b131065080a...

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH for-9.1] target/i386: Fix tss access size in switch_tss_ra

[Pierrick Bouvier]

On 8/19/24 00:40, Richard Henderson wrote:
> The two limit_max variables represent size - 1, just like the
> encoding in the GDT, thus the 'old' access was off by one.
> Access the minimal size of the new tss: the complete tss contains
> the iopb, which may be a larger block than the access api expects,
> and irrelevant because the iopb is not accessed during the
> switch itself.
> 
> Fixes: 8b131065080a ("target/i386/tcg: use X86Access for TSS access")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2511
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/i386/tcg/seg_helper.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c
> index bab552cd53..3b8fd827e1 100644
> --- a/target/i386/tcg/seg_helper.c
> +++ b/target/i386/tcg/seg_helper.c
> @@ -378,7 +378,7 @@ static int switch_tss_ra(CPUX86State *env, int tss_selector,
>   
>       /* X86Access avoids memory exceptions during the task switch */
>       mmu_index = cpu_mmu_index_kernel(env);
> -    access_prepare_mmu(&old, env, env->tr.base, old_tss_limit_max,
> +    access_prepare_mmu(&old, env, env->tr.base, old_tss_limit_max + 1,
>                          MMU_DATA_STORE, mmu_index, retaddr);
>   
>       if (source == SWITCH_TSS_CALL) {
> @@ -386,7 +386,8 @@ static int switch_tss_ra(CPUX86State *env, int tss_selector,
>           probe_access(env, tss_base, 2, MMU_DATA_STORE,
>                        mmu_index, retaddr);
>       }
> -    access_prepare_mmu(&new, env, tss_base, tss_limit,
> +    /* While true tss_limit may be larger, we don't access the iopb here. */
> +    access_prepare_mmu(&new, env, tss_base, tss_limit_max + 1,
>                          MMU_DATA_LOAD, mmu_index, retaddr);
>   
>       /* save the current state in the old TSS */

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>