[PATCH] target/ppc: fix unreachable code in do_ldst_quad()

Daniel Henrique Barboza posted 1 patch 1 year, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220720135723.1391598-1-danielhb413@gmail.com
Maintainers: "Cédric Le Goater" <clg@kaod.org>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>
There is a newer version of this series
target/ppc/translate/fixedpoint-impl.c.inc | 4 ++++
1 file changed, 4 insertions(+)
[PATCH] target/ppc: fix unreachable code in do_ldst_quad()
Posted by Daniel Henrique Barboza 1 year, 8 months ago
Coverity reports that commit fc34e81acd51 ("target/ppc: add macros to
check privilege level") turned the following code unreachable:

if (!prefixed && !(ctx->insns_flags2 & PPC2_LSQ_ISA207)) {
    /* lq and stq were privileged prior to V. 2.07 */
    REQUIRE_SV(ctx);

>>>     CID 1490757:  Control flow issues  (UNREACHABLE)
>>>     This code cannot be reached: "if (ctx->le_mode) {
    if (ctx->le_mode) {
        gen_align_no_le(ctx);
        return true;
    }
}

This happens because the macro REQUIRE_SV(), in CONFIG_USER_MODE, will
always result in a 'return true' statement.

Fix it by using "#if !defined(CONFIG_USER_ONLY)" to fold the code that
shouldn't be there if we're running in a non-privileged state. This is
also how the REQUIRE_SV() macro is being used in
storage-ctrl-impl.c.inc.

Fixes: Coverity CID 1490757
Fixes: fc34e81acd51 ("target/ppc: add macros to check privilege level")
Cc: Matheus Ferst <matheus.ferst@eldorado.org.br>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/translate/fixedpoint-impl.c.inc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
index db14d3bebc..4a32fac4f3 100644
--- a/target/ppc/translate/fixedpoint-impl.c.inc
+++ b/target/ppc/translate/fixedpoint-impl.c.inc
@@ -82,10 +82,14 @@ static bool do_ldst_quad(DisasContext *ctx, arg_D *a, bool store, bool prefixed)
         /* lq and stq were privileged prior to V. 2.07 */
         REQUIRE_SV(ctx);
 
+#if !defined(CONFIG_USER_ONLY)
         if (ctx->le_mode) {
             gen_align_no_le(ctx);
             return true;
         }
+#else
+    qemu_build_not_reached();
+#endif
     }
 
     if (!store && unlikely(a->ra == a->rt)) {
-- 
2.36.1
Re: [PATCH] target/ppc: fix unreachable code in do_ldst_quad()
Posted by Richard Henderson 1 year, 8 months ago
On 7/20/22 19:27, Daniel Henrique Barboza wrote:
> Coverity reports that commit fc34e81acd51 ("target/ppc: add macros to
> check privilege level") turned the following code unreachable:
> 
> if (!prefixed && !(ctx->insns_flags2 & PPC2_LSQ_ISA207)) {
>      /* lq and stq were privileged prior to V. 2.07 */
>      REQUIRE_SV(ctx);
> 
>>>>      CID 1490757:  Control flow issues  (UNREACHABLE)
>>>>      This code cannot be reached: "if (ctx->le_mode) {
>      if (ctx->le_mode) {
>          gen_align_no_le(ctx);
>          return true;
>      }
> }
> 
> This happens because the macro REQUIRE_SV(), in CONFIG_USER_MODE, will
> always result in a 'return true' statement.

I think adding ifdefs isn't fantastic.  This isn't actually fix a bug, so we *could* just 
mark this as ignore in Coverity.

If you wanted to clean this up, remove the implicit control flow from REQUIRE_* and turn 
the macros into pure predicates, so that you get

     if (REQUIRE_SV(ctx)) {
         return true;
     }
     if (ctx->le_mode) {
         ...
     }


r~
Re: [PATCH] target/ppc: fix unreachable code in do_ldst_quad()
Posted by Víctor Colombo 1 year, 8 months ago
On 20/07/2022 10:57, Daniel Henrique Barboza wrote:
> Coverity reports that commit fc34e81acd51 ("target/ppc: add macros to
> check privilege level") turned the following code unreachable:
> 
> if (!prefixed && !(ctx->insns_flags2 & PPC2_LSQ_ISA207)) {
>      /* lq and stq were privileged prior to V. 2.07 */
>      REQUIRE_SV(ctx);
> 
>>>>      CID 1490757:  Control flow issues  (UNREACHABLE)
>>>>      This code cannot be reached: "if (ctx->le_mode) {
>      if (ctx->le_mode) {
>          gen_align_no_le(ctx);
>          return true;
>      }
> }
> 
> This happens because the macro REQUIRE_SV(), in CONFIG_USER_MODE, will
> always result in a 'return true' statement.
> 
> Fix it by using "#if !defined(CONFIG_USER_ONLY)" to fold the code that
> shouldn't be there if we're running in a non-privileged state. This is
> also how the REQUIRE_SV() macro is being used in
> storage-ctrl-impl.c.inc.
> 
> Fixes: Coverity CID 1490757
> Fixes: fc34e81acd51 ("target/ppc: add macros to check privilege level")
> Cc: Matheus Ferst <matheus.ferst@eldorado.org.br>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   target/ppc/translate/fixedpoint-impl.c.inc | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
> index db14d3bebc..4a32fac4f3 100644
> --- a/target/ppc/translate/fixedpoint-impl.c.inc
> +++ b/target/ppc/translate/fixedpoint-impl.c.inc
> @@ -82,10 +82,14 @@ static bool do_ldst_quad(DisasContext *ctx, arg_D *a, bool store, bool prefixed)
>           /* lq and stq were privileged prior to V. 2.07 */
>           REQUIRE_SV(ctx);
> 
> +#if !defined(CONFIG_USER_ONLY)
>           if (ctx->le_mode) {
>               gen_align_no_le(ctx);
>               return true;
>           }
> +#else
> +    qemu_build_not_reached();

nit: I think the indentation here is off by 1 level (missing 4 spaces)?

> +#endif
>       }
> 
>       if (!store && unlikely(a->ra == a->rt)) {
> --
> 2.36.1
> 
> 
Reviewed-by: Víctor Colombo <victor.colombo@eldorado.org.br>

-- 
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>