Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode

[Peter Maydell]

Gave up pinging for i386 maintainers; will take this via target-arm.next.

thanks
-- PMM


On Mon, 15 Nov 2021 at 14:38, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Ping^4. Who is collecting target/i386 patches these days ?
>
> -- PMM
>
> On Mon, 1 Nov 2021 at 16:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > Ping^3, now 2 months after patch posted and reviewed...
> >
> > -- PMM
> >
> > On Mon, 27 Sept 2021 at 11:03, Peter Maydell <peter.maydell@linaro.org> 
> > wrote:
> > >
> > > Ping^2 !
> > >
> > > thanks
> > > -- PMM
> > >
> > > On Mon, 13 Sept 2021 at 13:34, Peter Maydell <peter.maydell@linaro.org> 
> > > wrote:
> > > >
> > > > Ping? (this has been reviewed)
> > > >
> > > > thanks
> > > > -- PMM
> > > >
> > > > On Wed, 1 Sept 2021 at 15:10, Peter Maydell <peter.maydell@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.maydell@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