Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Aurelien Jarno]

On Mon, Sep 15, 2008 at 10:38:34AM +0900, Shin-ichiro KAWASAKI wrote:
> Blue Swirl wrote:
>> On 9/14/08, Shin-ichiro KAWASAKI <address@hidden> wrote:
>>> Thank you for the comment!
>>>
>>>  Blue Swirl wrote:
>>>
>>>> On 9/14/08, Shin-ichiro KAWASAKI <address@hidden> wrote:
>>>>
>>>>> This patch adds check for all SH4 instructions which are
>>>>>  executed only in privileged mode.
>>>>>
>>>> The checks get the privileged mode status from translation context. In
>>>> theory, the same TB code block could be used in unprivileged and
>>>> privileged mode, so the status that was true at translation time may
>>>> no longer be correct at execution time. Of course normally kernel code
>>>> is not visible or executable to user processes.
>>>>
>>>  As you say, this patch has the restriction that you pointed out : the
>>>  generated TB cannot used for both unprivileged and privileged.
>>
>> Qemu will happily use the same TB for both modes, if the TB flags
>> match (cpu-exec.c):
>>
>>     if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
>>                  tb->flags != flags)) {
>>         tb = tb_find_slow(pc, cs_base, flags);
>>     }
>
> Thanks!  I have not understood this point.  You mean,
> the main problem is that the one TB can be reused for both modes, and to 
> check if new translation for the TB is necessary or not,
> flags should contain enough information taken from CPU status.
>
>>>  I guess the codes generated by tcg_gen_qemu_st/ld() have the same
>>>  restriction, because those tcg_gen functions take the argument QEMU memory
>>>  index flags, which is decided at translation time.  If it is true, the
>>>  restriction might be allowed for privilege check.
>>
>> The loads and stores have the same problem, the generated code assumes
>> that the privilege mode stays the same as what it was during
>> translation.
>
> And the other problem is that loads/stores instructions (and privilege
> instructions), cannot follow some CPU status changes within one TB.
> This problem will be left considering the trade off with performance.
>
>>>> The TB flags are handled in cpu-exec.c:tb_find_fast(). If I understand
>>>> the SH part correctly, the flags copied from env->flags don't contain
>>>> the privileged mode bits, isn't that in env->sr & SR_MD?
>>>>
>>>  Right.  In
>>> target-sh4/translate.c:get_intermediate_code_internal(),
>>>  the value env->sr & SR_MD used to set ctx->memidx.
>>>  We can use ctx->memidx to check the privileged mode at translation time,
>>>  and can use env->sr to check at execution time.  Both implementation
>>>  can be done, I guess.
>>
>> But ctx->memidx value will be accurate only if the TB flags contain
>> the SR_MD bit. Then if the bit is different, a new TB will be
>> generated using ctx-memidx that reflects the SR_MD bit.
>
> I see.  I made a patch to let TB flags contain SR_MD bit.
> At that time, I reviewed any other status of CPU are referred at
> translation time.  I found not only SR_MD, some other bits are
> referred also : e.g. bits in floating point control registers.
> The patch attached to this mail copy such bits into TB flags too.
> I hope it reflect your advise enough, doesn't it?


Applied, thanks!

> Thanks!
>
> regards,
> Shin-ichiro KAWASAKI
>
>
> Index: trunk/target-sh4/translate.c
> ===================================================================
> --- trunk/target-sh4/translate.c      (revision 5205)
> +++ trunk/target-sh4/translate.c      (working copy)
> @@ -48,6 +48,12 @@
>     int singlestep_enabled;
> } DisasContext;
>
> +#if defined(CONFIG_USER_ONLY)
> +#define IS_USER(ctx) 1
> +#else
> +#define IS_USER(ctx) (!(ctx->sr & SR_MD))
> +#endif
> +
> enum {
>     BS_NONE     = 0, /* We go out of the TB without reaching a branch or an
>                       * exception condition
> @@ -448,6 +454,13 @@
>   {tcg_gen_helper_0_0(helper_raise_slot_illegal_instruction); ctx->bstate = 
> BS_EXCP; \
>    return;}
>
> +#define CHECK_PRIVILEGED                                      \
> +  if (IS_USER(ctx)) {                                         \
> +      tcg_gen_helper_0_0(helper_raise_illegal_instruction);   \
> +      ctx->bstate = BS_EXCP;                                  \
> +      return;                                                 \
> +  }
> +
> void _decode_opc(DisasContext * ctx)
> {
> #if 0
> @@ -474,13 +487,11 @@
>       gen_clr_t();
>       return;
>     case 0x0038:              /* ldtlb */
> -#if defined(CONFIG_USER_ONLY)
> -     assert(0);              /* XXXXX */
> -#else
> +        CHECK_PRIVILEGED
>       tcg_gen_helper_0_0(helper_ldtlb);
> -#endif
>       return;
>     case 0x002b:              /* rte */
> +        CHECK_PRIVILEGED
>       CHECK_NOT_DELAY_SLOT
>       tcg_gen_mov_i32(cpu_sr, cpu_ssr);
>       tcg_gen_mov_i32(cpu_delayed_pc, cpu_spc);
> @@ -504,12 +515,8 @@
>     case 0x0009:              /* nop */
>       return;
>     case 0x001b:              /* sleep */
> -     if (ctx->memidx) {
> -             tcg_gen_helper_0_0(helper_sleep);
> -     } else {
> -             tcg_gen_helper_0_0(helper_raise_illegal_instruction);
> -             ctx->bstate = BS_EXCP;
> -     }
> +        CHECK_PRIVILEGED
> +     tcg_gen_helper_0_1(helper_sleep, tcg_const_i32(ctx->pc + 2));
>       return;
>     }
>
> @@ -1350,16 +1357,20 @@
>
>     switch (ctx->opcode & 0xf08f) {
>     case 0x408e:              /* ldc Rm,Rn_BANK */
> +        CHECK_PRIVILEGED
>       tcg_gen_mov_i32(ALTREG(B6_4), REG(B11_8));
>       return;
>     case 0x4087:              /* ldc.l @Rm+,Rn_BANK */
> +        CHECK_PRIVILEGED
>       tcg_gen_qemu_ld32s(ALTREG(B6_4), REG(B11_8), ctx->memidx);
>       tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);
>       return;
>     case 0x0082:              /* stc Rm_BANK,Rn */
> +        CHECK_PRIVILEGED
>       tcg_gen_mov_i32(REG(B11_8), ALTREG(B6_4));
>       return;
>     case 0x4083:              /* stc.l Rm_BANK,@-Rn */
> +        CHECK_PRIVILEGED
>       {
>           TCGv addr = tcg_temp_new(TCG_TYPE_I32);
>           tcg_gen_subi_i32(addr, REG(B11_8), 4);
> @@ -1407,11 +1418,13 @@
>       ctx->flags |= DELAY_SLOT;
>       ctx->delayed_pc = (uint32_t) - 1;
>       return;
> -    case 0x400e:             /* lds Rm,SR */
> +    case 0x400e:             /* ldc Rm,SR */
> +        CHECK_PRIVILEGED
>       tcg_gen_andi_i32(cpu_sr, REG(B11_8), 0x700083f3);
>       ctx->bstate = BS_STOP;
>       return;
> -    case 0x4007:             /* lds.l @Rm+,SR */
> +    case 0x4007:             /* ldc.l @Rm+,SR */
> +        CHECK_PRIVILEGED
>       {
>           TCGv val = tcg_temp_new(TCG_TYPE_I32);
>           tcg_gen_qemu_ld32s(val, REG(B11_8), ctx->memidx);
> @@ -1421,10 +1434,12 @@
>           ctx->bstate = BS_STOP;
>       }
>       return;
> -    case 0x0002:             /* sts SR,Rn */
> +    case 0x0002:             /* stc SR,Rn */
> +        CHECK_PRIVILEGED
>       tcg_gen_mov_i32(REG(B11_8), cpu_sr);
>       return;
> -    case 0x4003:             /* sts SR,@-Rn */
> +    case 0x4003:             /* stc SR,@-Rn */
> +        CHECK_PRIVILEGED
>       {
>           TCGv addr = tcg_temp_new(TCG_TYPE_I32);
>           tcg_gen_subi_i32(addr, REG(B11_8), 4);
> @@ -1433,18 +1448,22 @@
>           tcg_gen_subi_i32(REG(B11_8), REG(B11_8), 4);
>       }
>       return;
> -#define LDST(reg,ldnum,ldpnum,stnum,stpnum)                  \
> +#define LDST(reg,ldnum,ldpnum,stnum,stpnum,prechk)           \
>   case ldnum:                                                 \
> +    prechk                                                           \
>     tcg_gen_mov_i32 (cpu_##reg, REG(B11_8));                  \
>     return;                                                   \
>   case ldpnum:                                                        \
> +    prechk                                                           \
>     tcg_gen_qemu_ld32s (cpu_##reg, REG(B11_8), ctx->memidx);  \
>     tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);              \
>     return;                                                   \
>   case stnum:                                                 \
> +    prechk                                                           \
>     tcg_gen_mov_i32 (REG(B11_8), cpu_##reg);                  \
>     return;                                                   \
>   case stpnum:                                                        \
> +    prechk                                                           \
>     {                                                         \
>       TCGv addr = tcg_temp_new(TCG_TYPE_I32);                 \
>       tcg_gen_subi_i32(addr, REG(B11_8), 4);                  \
> @@ -1453,15 +1472,15 @@
>       tcg_gen_subi_i32(REG(B11_8), REG(B11_8), 4);            \
>     }                                                         \
>     return;
> -     LDST(gbr,  0x401e, 0x4017, 0x0012, 0x4013)
> -     LDST(vbr,  0x402e, 0x4027, 0x0022, 0x4023)
> -     LDST(ssr,  0x403e, 0x4037, 0x0032, 0x4033)
> -     LDST(spc,  0x404e, 0x4047, 0x0042, 0x4043)
> -     LDST(dbr,  0x40fa, 0x40f6, 0x00fa, 0x40f2)
> -     LDST(mach, 0x400a, 0x4006, 0x000a, 0x4002)
> -     LDST(macl, 0x401a, 0x4016, 0x001a, 0x4012)
> -     LDST(pr,   0x402a, 0x4026, 0x002a, 0x4022)
> -     LDST(fpul, 0x405a, 0x4056, 0x005a, 0x4052)
> +     LDST(gbr,  0x401e, 0x4017, 0x0012, 0x4013, {})
> +     LDST(vbr,  0x402e, 0x4027, 0x0022, 0x4023, CHECK_PRIVILEGED)
> +     LDST(ssr,  0x403e, 0x4037, 0x0032, 0x4033, CHECK_PRIVILEGED)
> +     LDST(spc,  0x404e, 0x4047, 0x0042, 0x4043, CHECK_PRIVILEGED)
> +     LDST(dbr,  0x40fa, 0x40f6, 0x00fa, 0x40f2, CHECK_PRIVILEGED)
> +     LDST(mach, 0x400a, 0x4006, 0x000a, 0x4002, {})
> +     LDST(macl, 0x401a, 0x4016, 0x001a, 0x4012, {})
> +     LDST(pr,   0x402a, 0x4026, 0x002a, 0x4022, {})
> +     LDST(fpul, 0x405a, 0x4056, 0x005a, 0x4052, {})
>     case 0x406a:              /* lds Rm,FPSCR */
>       tcg_gen_helper_0_1(helper_ld_fpscr, REG(B11_8));
>       ctx->bstate = BS_STOP;
> Index: trunk/cpu-exec.c
> ===================================================================
> --- trunk/cpu-exec.c  (revision 5205)
> +++ trunk/cpu-exec.c  (working copy)
> @@ -209,7 +209,10 @@
>     cs_base = 0;
>     pc = env->pc;
> #elif defined(TARGET_SH4)
> -    flags = env->flags;
> +    flags = (env->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL
> +                    | DELAY_SLOT_TRUE | DELAY_SLOT_CLEARME))   /* Bits  0- 3 
> */
> +            | (env->fpscr & (FPSCR_FR | FPSCR_SZ | FPSCR_PR))  /* Bits 19-21 
> */
> +            | (env->sr & (SR_MD | SR_RB));                     /* Bits 29-30 
> */
>     cs_base = 0;
>     pc = env->pc;
> #elif defined(TARGET_ALPHA)
>
>
>

-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net