[PATCH v3 0/3] accel/tcg: Make sure that tb->size != 0 after translation

[PATCH v3 0/3] accel/tcg: Make sure that tb->size != 0 after translation

[Ilya Leoshkevich]

If arch-specific code generates a translation block of size 0,
tb_gen_code() may generate a spurious exception.

Fix s390x (patch 1) and ARM (patch 2) and add an assertion in order to
catch such situations early (patch 3).

v1: https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02037.html
v1 -> v2: Fix target/s390x instead of trying to tolerate tb->size == 0
          in tb_gen_code().

v2: https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02101.html
v2 -> v3: Split the common code change into a separate patch, add the
          ARM patch in order to fix
          https://gitlab.com/cohuck/qemu/-/jobs/1178409450

Ilya Leoshkevich (3):
  target/s390x: Fix translation exception on illegal instruction
  target/arm: Make sure that commpage's tb->size != 0
  accel/tcg: Assert that tb->size != 0 after translation

 accel/tcg/translate-all.c |  1 +
 target/arm/translate.c    |  1 +
 target/s390x/translate.c  | 16 +++++++++++-----
 3 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.29.2

[PATCH v3 1/3] target/s390x: Fix translation exception on illegal instruction

[Ilya Leoshkevich]

Hitting an uretprobe in a s390x TCG guest causes a SIGSEGV. What
happens is:

* uretprobe maps a userspace page containing an invalid instruction.
* uretprobe replaces the target function's return address with the
  address of that page.
* When tb_gen_code() is called on that page, tb->size ends up being 0
  (because the page starts with the invalid instruction), which causes
  virt_page2 to point to the previous page.
* The previous page is not mapped, so this causes a spurious
  translation exception.

tb->size must never be 0: even if there is an illegal instruction, the
instruction bytes that have been looked at must count towards tb->size.
So adjust s390x's translate_one() to act this way for both illegal
instructions and instructions that are known to generate exceptions.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/translate.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 4f953ddfba..e243624d2a 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -6412,7 +6412,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
         qemu_log_mask(LOG_UNIMP, "unimplemented opcode 0x%02x%02x\n",
                       s->fields.op, s->fields.op2);
         gen_illegal_opcode(s);
-        return DISAS_NORETURN;
+        ret = DISAS_NORETURN;
+        goto out;
     }
 
 #ifndef CONFIG_USER_ONLY
@@ -6428,7 +6429,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
         /* privileged instruction */
         if ((s->base.tb->flags & FLAG_MASK_PSTATE) && (insn->flags & IF_PRIV)) {
             gen_program_exception(s, PGM_PRIVILEGED);
-            return DISAS_NORETURN;
+            ret = DISAS_NORETURN;
+            goto out;
         }
 
         /* if AFP is not enabled, instructions and registers are forbidden */
@@ -6455,7 +6457,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
             }
             if (dxc) {
                 gen_data_exception(dxc);
-                return DISAS_NORETURN;
+                ret = DISAS_NORETURN;
+                goto out;
             }
         }
 
@@ -6463,7 +6466,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
         if (insn->flags & IF_VEC) {
             if (!((s->base.tb->flags & FLAG_MASK_VECTOR))) {
                 gen_data_exception(0xfe);
-                return DISAS_NORETURN;
+                ret = DISAS_NORETURN;
+                goto out;
             }
         }
 
@@ -6484,7 +6488,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
             (insn->spec & SPEC_r1_f128 && !is_fp_pair(get_field(s, r1))) ||
             (insn->spec & SPEC_r2_f128 && !is_fp_pair(get_field(s, r2)))) {
             gen_program_exception(s, PGM_SPECIFICATION);
-            return DISAS_NORETURN;
+            ret = DISAS_NORETURN;
+            goto out;
         }
     }
 
@@ -6544,6 +6549,7 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
     }
 #endif
 
+out:
     /* Advance to the next instruction.  */
     s->base.pc_next = s->pc_tmp;
     return ret;
-- 
2.29.2

Re: [PATCH v3 1/3] target/s390x: Fix translation exception on illegal instruction

[David Hildenbrand]

On 14.04.21 15:41, Ilya Leoshkevich wrote:
> Hitting an uretprobe in a s390x TCG guest causes a SIGSEGV. What
> happens is:
> 
> * uretprobe maps a userspace page containing an invalid instruction.
> * uretprobe replaces the target function's return address with the
>    address of that page.
> * When tb_gen_code() is called on that page, tb->size ends up being 0
>    (because the page starts with the invalid instruction), which causes
>    virt_page2 to point to the previous page.
> * The previous page is not mapped, so this causes a spurious
>    translation exception.
> 
> tb->size must never be 0: even if there is an illegal instruction, the
> instruction bytes that have been looked at must count towards tb->size.
> So adjust s390x's translate_one() to act this way for both illegal
> instructions and instructions that are known to generate exceptions.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/translate.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 4f953ddfba..e243624d2a 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -6412,7 +6412,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>           qemu_log_mask(LOG_UNIMP, "unimplemented opcode 0x%02x%02x\n",
>                         s->fields.op, s->fields.op2);
>           gen_illegal_opcode(s);
> -        return DISAS_NORETURN;
> +        ret = DISAS_NORETURN;
> +        goto out;
>       }
>   
>   #ifndef CONFIG_USER_ONLY
> @@ -6428,7 +6429,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>           /* privileged instruction */
>           if ((s->base.tb->flags & FLAG_MASK_PSTATE) && (insn->flags & IF_PRIV)) {
>               gen_program_exception(s, PGM_PRIVILEGED);
> -            return DISAS_NORETURN;
> +            ret = DISAS_NORETURN;
> +            goto out;
>           }
>   
>           /* if AFP is not enabled, instructions and registers are forbidden */
> @@ -6455,7 +6457,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>               }
>               if (dxc) {
>                   gen_data_exception(dxc);
> -                return DISAS_NORETURN;
> +                ret = DISAS_NORETURN;
> +                goto out;
>               }
>           }
>   
> @@ -6463,7 +6466,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>           if (insn->flags & IF_VEC) {
>               if (!((s->base.tb->flags & FLAG_MASK_VECTOR))) {
>                   gen_data_exception(0xfe);
> -                return DISAS_NORETURN;
> +                ret = DISAS_NORETURN;
> +                goto out;
>               }
>           }
>   
> @@ -6484,7 +6488,8 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>               (insn->spec & SPEC_r1_f128 && !is_fp_pair(get_field(s, r1))) ||
>               (insn->spec & SPEC_r2_f128 && !is_fp_pair(get_field(s, r2)))) {
>               gen_program_exception(s, PGM_SPECIFICATION);
> -            return DISAS_NORETURN;
> +            ret = DISAS_NORETURN;
> +            goto out;
>           }
>       }
>   
> @@ -6544,6 +6549,7 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>       }
>   #endif
>   
> +out:
>       /* Advance to the next instruction.  */
>       s->base.pc_next = s->pc_tmp;
>       return ret;
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v3 2/3] target/arm: Make sure that commpage's tb->size != 0

[Ilya Leoshkevich]

tb_gen_code() assumes that tb->size must never be zero, otherwise it
may produce spurious exceptions. For ARM this may happen when creating
a translation block for the commpage.

Fix by pretending that commpage translation blocks have at least one
instruction.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/arm/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 62b1c2081b..885f69b044 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9060,6 +9060,7 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     unsigned int insn;
 
     if (arm_pre_translate_insn(dc)) {
+        dc->base.pc_next += 4;
         return;
     }
 
-- 
2.29.2

[PATCH v3 3/3] accel/tcg: Assert that tb->size != 0 after translation

[Ilya Leoshkevich]

If arch-specific code generates a translation block of size 0,
tb_gen_code() may generate a spurious exception. Add an assertion in
order to catch such situations early.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 accel/tcg/translate-all.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index ba6ab09790..93b2dae112 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1913,6 +1913,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 
     tcg_ctx->cpu = env_cpu(env);
     gen_intermediate_code(cpu, tb, max_insns);
+    assert(tb->size != 0);
     tcg_ctx->cpu = NULL;
     max_insns = tb->icount;
 
-- 
2.29.2

Re: [PATCH v3 3/3] accel/tcg: Assert that tb->size != 0 after translation

[David Hildenbrand]

On 14.04.21 15:41, Ilya Leoshkevich wrote:
> If arch-specific code generates a translation block of size 0,
> tb_gen_code() may generate a spurious exception. Add an assertion in
> order to catch such situations early.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   accel/tcg/translate-all.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index ba6ab09790..93b2dae112 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1913,6 +1913,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>   
>       tcg_ctx->cpu = env_cpu(env);
>       gen_intermediate_code(cpu, tb, max_insns);
> +    assert(tb->size != 0);
>       tcg_ctx->cpu = NULL;
>       max_insns = tb->icount;
>   
> 

Did you double-check the xtensa issue?

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 3/3] accel/tcg: Assert that tb->size != 0 after translation

[Ilya Leoshkevich]

On Wed, 2021-04-14 at 16:48 +0200, David Hildenbrand wrote:
> On 14.04.21 15:41, Ilya Leoshkevich wrote:
> > If arch-specific code generates a translation block of size 0,
> > tb_gen_code() may generate a spurious exception. Add an assertion
> > in
> > order to catch such situations early.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   accel/tcg/translate-all.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> > index ba6ab09790..93b2dae112 100644
> > --- a/accel/tcg/translate-all.c
> > +++ b/accel/tcg/translate-all.c
> > @@ -1913,6 +1913,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> >   
> >       tcg_ctx->cpu = env_cpu(env);
> >       gen_intermediate_code(cpu, tb, max_insns);
> > +    assert(tb->size != 0);
> >       tcg_ctx->cpu = NULL;
> >       max_insns = tb->icount;
> >   
> > 
> 
> Did you double-check the xtensa issue?

Oh, I'm sorry, I completely forgot about that one. I just ran the
test locally, and apparently it fails because of this new assert, so
I'll have to write the 4th patch now. Thanks!

> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 

Re: [PATCH v3 3/3] accel/tcg: Assert that tb->size != 0 after translation

[Max Filippov]

On Wed, Apr 14, 2021 at 9:51 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> On Wed, 2021-04-14 at 16:48 +0200, David Hildenbrand wrote:
> > Did you double-check the xtensa issue?
>
> Oh, I'm sorry, I completely forgot about that one. I just ran the
> test locally, and apparently it fails because of this new assert, so
> I'll have to write the 4th patch now. Thanks!

Just curious, what xtensa issue?

-- 
Thanks.
-- Max

Re: [PATCH v3 3/3] accel/tcg: Assert that tb->size != 0 after translation

[Richard Henderson]

On 4/14/21 11:03 AM, Max Filippov wrote:
> On Wed, Apr 14, 2021 at 9:51 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>> On Wed, 2021-04-14 at 16:48 +0200, David Hildenbrand wrote:
>>> Did you double-check the xtensa issue?
>>
>> Oh, I'm sorry, I completely forgot about that one. I just ran the
>> test locally, and apparently it fails because of this new assert, so
>> I'll have to write the 4th patch now. Thanks!
> 
> Just curious, what xtensa issue?

Returning from xtensa_tr_translate_insn with tb->size == 0.

Basically, dc->base.pc_next needs to be incremented even for illegal 
instructions, preferably by the number of bytes consumed while determining that 
the insn is illegal.


r~

Re: [PATCH v3 3/3] accel/tcg: Assert that tb->size != 0 after translation

[Max Filippov]

On Wed, Apr 14, 2021 at 12:43 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/14/21 11:03 AM, Max Filippov wrote:
> > On Wed, Apr 14, 2021 at 9:51 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >> On Wed, 2021-04-14 at 16:48 +0200, David Hildenbrand wrote:
> >>> Did you double-check the xtensa issue?
> >>
> >> Oh, I'm sorry, I completely forgot about that one. I just ran the
> >> test locally, and apparently it fails because of this new assert, so
> >> I'll have to write the 4th patch now. Thanks!
> >
> > Just curious, what xtensa issue?
>
> Returning from xtensa_tr_translate_insn with tb->size == 0.
>
> Basically, dc->base.pc_next needs to be incremented even for illegal
> instructions, preferably by the number of bytes consumed while determining that
> the insn is illegal.

I see a few places where target/xtensa may do that. E.g. it does that on entry
to an exception handler to allow for debugging its first instruction.
No guest code
is consumed to make this decision, would size 1 work in that case?
I'll take a look.

-- 
Thanks.
-- Max

Re: [PATCH v3 3/3] accel/tcg: Assert that tb->size != 0 after translation

[Ilya Leoshkevich]

On Wed, 2021-04-14 at 18:23 -0700, Max Filippov wrote:
> On Wed, Apr 14, 2021 at 12:43 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > 
> > On 4/14/21 11:03 AM, Max Filippov wrote:
> > > On Wed, Apr 14, 2021 at 9:51 AM Ilya Leoshkevich <
> > > iii@linux.ibm.com> wrote:
> > > > On Wed, 2021-04-14 at 16:48 +0200, David Hildenbrand wrote:
> > > > > Did you double-check the xtensa issue?
> > > > 
> > > > Oh, I'm sorry, I completely forgot about that one. I just ran the
> > > > test locally, and apparently it fails because of this new assert,
> > > > so
> > > > I'll have to write the 4th patch now. Thanks!
> > > 
> > > Just curious, what xtensa issue?
> > 
> > Returning from xtensa_tr_translate_insn with tb->size == 0.
> > 
> > Basically, dc->base.pc_next needs to be incremented even for illegal
> > instructions, preferably by the number of bytes consumed while
> > determining that
> > the insn is illegal.
> 
> I see a few places where target/xtensa may do that. E.g. it does that
> on entry
> to an exception handler to allow for debugging its first instruction.
> No guest code
> is consumed to make this decision, would size 1 work in that case?
> I'll take a look.

Returning 1 was my idea as well. Here is what seems to fix the failure
and what I'm currently testing locally:

--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -917,6 +917,8 @@ static void disas_xtensa_insn(CPUXtensaState *env,
DisasContext *dc)
                       "unknown instruction length (pc = %08x)\n",
                       dc->pc);
         gen_exception_cause(dc, ILLEGAL_INSTRUCTION_CAUSE);
+        dc->base.pc_next = dc->pc + 1;
+        dc->base.is_jmp = DISAS_NORETURN;
         return;
     }
 
@@ -1274,11 +1276,13 @@ static void
xtensa_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     if ((tb_cflags(dc->base.tb) & CF_USE_ICOUNT)
         && (dc->base.tb->flags & XTENSA_TBFLAG_YIELD)) {
         gen_exception(dc, EXCP_YIELD);
+        dc->base.pc_next = dc->pc + 1;
         dc->base.is_jmp = DISAS_NORETURN;
         return;
     }
     if (dc->base.tb->flags & XTENSA_TBFLAG_EXCEPTION) {
         gen_exception(dc, EXCP_DEBUG);
+        dc->base.pc_next = dc->pc + 1;
         dc->base.is_jmp = DISAS_NORETURN;
         return;
     }

Re: [PATCH v3 3/3] accel/tcg: Assert that tb->size != 0 after translation

[Richard Henderson]

On 4/14/21 6:23 PM, Max Filippov wrote:
> I see a few places where target/xtensa may do that. E.g. it does that on entry
> to an exception handler to allow for debugging its first instruction.
> No guest code
> is consumed to make this decision, would size 1 work in that case?
> I'll take a look.

Yes, any positive value will do.


r~

Re: [PATCH v3 3/3] accel/tcg: Assert that tb->size != 0 after translation

[Peter Maydell]

On Thu, 15 Apr 2021 at 02:24, Max Filippov <jcmvbkbc@gmail.com> wrote:
> I see a few places where target/xtensa may do that. E.g. it does that on entry
> to an exception handler to allow for debugging its first instruction.

That should now be handled by the common code, I think (see commits
a7ba744f4082ab and ba3c35d9c402636) -- does xtensa still need to
special case this ?

thanks
-- PMM

Re: [PATCH v3 3/3] accel/tcg: Assert that tb->size != 0 after translation

[Max Filippov]

On Thu, Apr 15, 2021 at 8:03 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 15 Apr 2021 at 02:24, Max Filippov <jcmvbkbc@gmail.com> wrote:
> > I see a few places where target/xtensa may do that. E.g. it does that on entry
> > to an exception handler to allow for debugging its first instruction.
>
> That should now be handled by the common code, I think (see commits
> a7ba744f4082ab and ba3c35d9c402636) -- does xtensa still need to
> special case this ?

Thanks for letting me know. It now stops there twice, so no, special casing
is no longer needed. I'll send a patch.

-- 
Thanks.
-- Max