Ping? (this has been reviewed) thanks -- PMM
On Wed, 1 Sept 2021 at 15:10, Peter Maydell <peter.mayd...@linaro.org> wrote: > > In the SSE decode function gen_sse(), we combine a byte > 'b' and a value 'b1' which can be [0..3], and switch on them: > b |= (b1 << 8); > switch (b) { > ... > default: > unknown_op: > gen_unknown_opcode(env, s); > return; > } > > In three cases inside this switch, we were then also checking for > "if (b1 >= 2) { goto unknown_op; }". > However, this can never happen, because the 'case' values in each place > are 0x0nn or 0x1nn and the switch will have directed the b1 == (2, 3) > cases to the default already. > > This check was added in commit c045af25a52e9 in 2010; the added code > was unnecessary then as well, and was apparently intended only to > ensure that we never accidentally ended up indexing off the end > of an sse_op_table with only 2 entries as a result of future bugs > in the decode logic. > > Change the checks to assert() instead, and make sure they're always > immediately before the array access they are protecting. > > Fixes: Coverity CID 1460207 > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> > --- > v1->v2: use assert() rather than just deleting the if()s > > target/i386/tcg/translate.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c > index aacb605eee4..a4fee5e445d 100644 > --- a/target/i386/tcg/translate.c > +++ b/target/i386/tcg/translate.c > @@ -3521,9 +3521,6 @@ static void gen_sse(CPUX86State *env, DisasContext *s, > int b, > case 0x171: /* shift xmm, im */ > case 0x172: > case 0x173: > - if (b1 >= 2) { > - goto unknown_op; > - } > val = x86_ldub_code(env, s); > if (is_xmm) { > tcg_gen_movi_tl(s->T0, val); > @@ -3542,6 +3539,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s, > int b, > offsetof(CPUX86State, mmx_t0.MMX_L(1))); > op1_offset = offsetof(CPUX86State,mmx_t0); > } > + assert(b1 < 2); > sse_fn_epp = sse_op_table2[((b - 1) & 3) * 8 + > (((modrm >> 3)) & 7)][b1]; > if (!sse_fn_epp) { > @@ -3772,10 +3770,8 @@ static void gen_sse(CPUX86State *env, DisasContext *s, > int b, > rm = modrm & 7; > reg = ((modrm >> 3) & 7) | REX_R(s); > mod = (modrm >> 6) & 3; > - if (b1 >= 2) { > - goto unknown_op; > - } > > + assert(b1 < 2); > sse_fn_epp = sse_op_table6[b].op[b1]; > if (!sse_fn_epp) { > goto unknown_op; > @@ -4202,10 +4198,8 @@ static void gen_sse(CPUX86State *env, DisasContext *s, > int b, > rm = modrm & 7; > reg = ((modrm >> 3) & 7) | REX_R(s); > mod = (modrm >> 6) & 3; > - if (b1 >= 2) { > - goto unknown_op; > - } > > + assert(b1 < 2); > sse_fn_eppi = sse_op_table7[b].op[b1]; > if (!sse_fn_eppi) { > goto unknown_op; > -- > 2.20.1