| Message ID | 20210511101951.165287-25-alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v3,01/42] target/riscv: Remove privilege v1.9 specific CSR related code | expand |
On Tue, 11 May 2021 at 11:21, Alistair Francis <alistair.francis@wdc.com> wrote: > > From: Hou Weiying <weiying_hou@outlook.com> > > This commit adds support for ePMP v0.9.1. > > The ePMP spec can be found in: > https://docs.google.com/document/d/1Mh_aiHYxemL0umN3GTTw8vsbmzHZ_nxZXgjgOUzbvc8 > > Signed-off-by: Hongzheng-Li <Ethan.Lee.QNL@gmail.com> > Signed-off-by: Hou Weiying <weiying_hou@outlook.com> > Signed-off-by: Myriad-Dreamin <camiyoru@gmail.com> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > Message-id: fef23b885f9649a4d54e7c98b168bdec5d297bb1.1618812899.git.alistair.francis@wdc.com > [ Changes by AF: > - Rebase on master > - Update to latest spec > - Use a switch case to handle ePMP MML permissions > - Fix a few bugs > ] > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> Hi; this code confuses Coverity into thinking that the pmp_hart_has_privs() function might read the value pointed to by 'allowed_privs' when it is uninitialized (CID 1453108): > @@ -294,13 +351,94 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr, > pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg); > > /* > - * If the PMP entry is not off and the address is in range, do the priv > - * check > + * Convert the PMP permissions to match the truth table in the > + * ePMP spec. > */ > + const uint8_t epmp_operation = > + ((env->pmp_state.pmp[i].cfg_reg & PMP_LOCK) >> 4) | > + ((env->pmp_state.pmp[i].cfg_reg & PMP_READ) << 2) | > + (env->pmp_state.pmp[i].cfg_reg & PMP_WRITE) | > + ((env->pmp_state.pmp[i].cfg_reg & PMP_EXEC) >> 2); Here we construct a value which can only be in the range [0,15], but we do it in a way that Coverity isn't clever enough to figure out... > + > if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) { > - *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; > - if ((mode != PRV_M) || pmp_is_locked(env, i)) { > - *allowed_privs &= env->pmp_state.pmp[i].cfg_reg; > + /* > + * If the PMP entry is not off and the address is in range, > + * do the priv check > + */ > + if (!MSECCFG_MML_ISSET(env)) { > + /* > + * If mseccfg.MML Bit is not set, do pmp priv check > + * This will always apply to regular PMP. > + */ > + *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; > + if ((mode != PRV_M) || pmp_is_locked(env, i)) { > + *allowed_privs &= env->pmp_state.pmp[i].cfg_reg; > + } > + } else { > + /* > + * If mseccfg.MML Bit set, do the enhanced pmp priv check > + */ > + if (mode == PRV_M) { > + switch (epmp_operation) { > + case 0: > + case 1: > + case 4: > + case 5: > + case 6: > + case 7: > + case 8: > + *allowed_privs = 0; > + break; > + case 2: > + case 3: > + case 14: > + *allowed_privs = PMP_READ | PMP_WRITE; > + break; > + case 9: > + case 10: > + *allowed_privs = PMP_EXEC; > + break; > + case 11: > + case 13: > + *allowed_privs = PMP_READ | PMP_EXEC; > + break; > + case 12: > + case 15: > + *allowed_privs = PMP_READ; > + break; ...so coverity thinks that "via the 'default' case" is a valid flow of control in these switch() statements... > + } > + } else { > + switch (epmp_operation) { > + case 0: > + case 8: > + case 9: > + case 12: > + case 13: > + case 14: > + *allowed_privs = 0; > + break; > + case 1: > + case 10: > + case 11: > + *allowed_privs = PMP_EXEC; > + break; > + case 2: > + case 4: > + case 15: > + *allowed_privs = PMP_READ; > + break; > + case 3: > + case 6: > + *allowed_privs = PMP_READ | PMP_WRITE; > + break; > + case 5: > + *allowed_privs = PMP_READ | PMP_EXEC; > + break; > + case 7: > + *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; > + break; > + } > + } > } > > ret = ((privs & *allowed_privs) == privs); ...and that we can get to here without having ever set *allowed_privs. Adding default: g_assert_not_reached(); to both switches should clarify to both Coverity and human readers that the cases in the switch are a complete enumeration of the possibilities. thanks -- PMM
On Thu, May 20, 2021 at 11:51 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 11 May 2021 at 11:21, Alistair Francis <alistair.francis@wdc.com> wrote: > > > > From: Hou Weiying <weiying_hou@outlook.com> > > > > This commit adds support for ePMP v0.9.1. > > > > The ePMP spec can be found in: > > https://docs.google.com/document/d/1Mh_aiHYxemL0umN3GTTw8vsbmzHZ_nxZXgjgOUzbvc8 > > > > Signed-off-by: Hongzheng-Li <Ethan.Lee.QNL@gmail.com> > > Signed-off-by: Hou Weiying <weiying_hou@outlook.com> > > Signed-off-by: Myriad-Dreamin <camiyoru@gmail.com> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > Message-id: fef23b885f9649a4d54e7c98b168bdec5d297bb1.1618812899.git.alistair.francis@wdc.com > > [ Changes by AF: > > - Rebase on master > > - Update to latest spec > > - Use a switch case to handle ePMP MML permissions > > - Fix a few bugs > > ] > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > Hi; this code confuses Coverity into thinking that the pmp_hart_has_privs() > function might read the value pointed to by 'allowed_privs' when > it is uninitialized (CID 1453108): > > > > @@ -294,13 +351,94 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr, > > pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg); > > > > /* > > - * If the PMP entry is not off and the address is in range, do the priv > > - * check > > + * Convert the PMP permissions to match the truth table in the > > + * ePMP spec. > > */ > > + const uint8_t epmp_operation = > > + ((env->pmp_state.pmp[i].cfg_reg & PMP_LOCK) >> 4) | > > + ((env->pmp_state.pmp[i].cfg_reg & PMP_READ) << 2) | > > + (env->pmp_state.pmp[i].cfg_reg & PMP_WRITE) | > > + ((env->pmp_state.pmp[i].cfg_reg & PMP_EXEC) >> 2); > > Here we construct a value which can only be in the range [0,15], > but we do it in a way that Coverity isn't clever enough to figure out... > > > + > > if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) { > > - *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; > > - if ((mode != PRV_M) || pmp_is_locked(env, i)) { > > - *allowed_privs &= env->pmp_state.pmp[i].cfg_reg; > > + /* > > + * If the PMP entry is not off and the address is in range, > > + * do the priv check > > + */ > > + if (!MSECCFG_MML_ISSET(env)) { > > + /* > > + * If mseccfg.MML Bit is not set, do pmp priv check > > + * This will always apply to regular PMP. > > + */ > > + *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; > > + if ((mode != PRV_M) || pmp_is_locked(env, i)) { > > + *allowed_privs &= env->pmp_state.pmp[i].cfg_reg; > > + } > > + } else { > > + /* > > + * If mseccfg.MML Bit set, do the enhanced pmp priv check > > + */ > > + if (mode == PRV_M) { > > + switch (epmp_operation) { > > + case 0: > > + case 1: > > + case 4: > > + case 5: > > + case 6: > > + case 7: > > + case 8: > > + *allowed_privs = 0; > > + break; > > + case 2: > > + case 3: > > + case 14: > > + *allowed_privs = PMP_READ | PMP_WRITE; > > + break; > > + case 9: > > + case 10: > > + *allowed_privs = PMP_EXEC; > > + break; > > + case 11: > > + case 13: > > + *allowed_privs = PMP_READ | PMP_EXEC; > > + break; > > + case 12: > > + case 15: > > + *allowed_privs = PMP_READ; > > + break; > > ...so coverity thinks that "via the 'default' case" is a valid flow > of control in these switch() statements... > > > + } > > + } else { > > + switch (epmp_operation) { > > + case 0: > > + case 8: > > + case 9: > > + case 12: > > + case 13: > > + case 14: > > + *allowed_privs = 0; > > + break; > > + case 1: > > + case 10: > > + case 11: > > + *allowed_privs = PMP_EXEC; > > + break; > > + case 2: > > + case 4: > > + case 15: > > + *allowed_privs = PMP_READ; > > + break; > > + case 3: > > + case 6: > > + *allowed_privs = PMP_READ | PMP_WRITE; > > + break; > > + case 5: > > + *allowed_privs = PMP_READ | PMP_EXEC; > > + break; > > + case 7: > > + *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; > > + break; > > + } > > + } > > } > > > > ret = ((privs & *allowed_privs) == privs); > > ...and that we can get to here without having ever set *allowed_privs. > > > Adding > default: > g_assert_not_reached(); > > to both switches should clarify to both Coverity and human readers that > the cases in the switch are a complete enumeration of the possibilities. Thanks Peter, I'll send a fix. Alistair > > thanks > -- PMM
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index e35988eec2..e1f5776316 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -90,11 +90,42 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index) static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val) { if (pmp_index < MAX_RISCV_PMPS) { - if (!pmp_is_locked(env, pmp_index)) { - env->pmp_state.pmp[pmp_index].cfg_reg = val; - pmp_update_rule(env, pmp_index); + bool locked = true; + + if (riscv_feature(env, RISCV_FEATURE_EPMP)) { + /* mseccfg.RLB is set */ + if (MSECCFG_RLB_ISSET(env)) { + locked = false; + } + + /* mseccfg.MML is not set */ + if (!MSECCFG_MML_ISSET(env) && !pmp_is_locked(env, pmp_index)) { + locked = false; + } + + /* mseccfg.MML is set */ + if (MSECCFG_MML_ISSET(env)) { + /* not adding execute bit */ + if ((val & PMP_LOCK) != 0 && (val & PMP_EXEC) != PMP_EXEC) { + locked = false; + } + /* shared region and not adding X bit */ + if ((val & PMP_LOCK) != PMP_LOCK && + (val & 0x7) != (PMP_WRITE | PMP_EXEC)) { + locked = false; + } + } } else { + if (!pmp_is_locked(env, pmp_index)) { + locked = false; + } + } + + if (locked) { qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n"); + } else { + env->pmp_state.pmp[pmp_index].cfg_reg = val; + pmp_update_rule(env, pmp_index); } } else { qemu_log_mask(LOG_GUEST_ERROR, @@ -217,6 +248,32 @@ static bool pmp_hart_has_privs_default(CPURISCVState *env, target_ulong addr, { bool ret; + if (riscv_feature(env, RISCV_FEATURE_EPMP)) { + if (MSECCFG_MMWP_ISSET(env)) { + /* + * The Machine Mode Whitelist Policy (mseccfg.MMWP) is set + * so we default to deny all, even for M-mode. + */ + *allowed_privs = 0; + return false; + } else if (MSECCFG_MML_ISSET(env)) { + /* + * The Machine Mode Lockdown (mseccfg.MML) bit is set + * so we can only execute code in M-mode with an applicable + * rule. Other modes are disabled. + */ + if (mode == PRV_M && !(privs & PMP_EXEC)) { + ret = true; + *allowed_privs = PMP_READ | PMP_WRITE; + } else { + ret = false; + *allowed_privs = 0; + } + + return ret; + } + } + if ((!riscv_feature(env, RISCV_FEATURE_PMP)) || (mode == PRV_M)) { /* * Privileged spec v1.10 states if HW doesn't implement any PMP entry @@ -294,13 +351,94 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr, pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg); /* - * If the PMP entry is not off and the address is in range, do the priv - * check + * Convert the PMP permissions to match the truth table in the + * ePMP spec. */ + const uint8_t epmp_operation = + ((env->pmp_state.pmp[i].cfg_reg & PMP_LOCK) >> 4) | + ((env->pmp_state.pmp[i].cfg_reg & PMP_READ) << 2) | + (env->pmp_state.pmp[i].cfg_reg & PMP_WRITE) | + ((env->pmp_state.pmp[i].cfg_reg & PMP_EXEC) >> 2); + if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) { - *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; - if ((mode != PRV_M) || pmp_is_locked(env, i)) { - *allowed_privs &= env->pmp_state.pmp[i].cfg_reg; + /* + * If the PMP entry is not off and the address is in range, + * do the priv check + */ + if (!MSECCFG_MML_ISSET(env)) { + /* + * If mseccfg.MML Bit is not set, do pmp priv check + * This will always apply to regular PMP. + */ + *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; + if ((mode != PRV_M) || pmp_is_locked(env, i)) { + *allowed_privs &= env->pmp_state.pmp[i].cfg_reg; + } + } else { + /* + * If mseccfg.MML Bit set, do the enhanced pmp priv check + */ + if (mode == PRV_M) { + switch (epmp_operation) { + case 0: + case 1: + case 4: + case 5: + case 6: + case 7: + case 8: + *allowed_privs = 0; + break; + case 2: + case 3: + case 14: + *allowed_privs = PMP_READ | PMP_WRITE; + break; + case 9: + case 10: + *allowed_privs = PMP_EXEC; + break; + case 11: + case 13: + *allowed_privs = PMP_READ | PMP_EXEC; + break; + case 12: + case 15: + *allowed_privs = PMP_READ; + break; + } + } else { + switch (epmp_operation) { + case 0: + case 8: + case 9: + case 12: + case 13: + case 14: + *allowed_privs = 0; + break; + case 1: + case 10: + case 11: + *allowed_privs = PMP_EXEC; + break; + case 2: + case 4: + case 15: + *allowed_privs = PMP_READ; + break; + case 3: + case 6: + *allowed_privs = PMP_READ | PMP_WRITE; + break; + case 5: + *allowed_privs = PMP_READ | PMP_EXEC; + break; + case 7: + *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; + break; + } + } } ret = ((privs & *allowed_privs) == privs);