Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
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 wrote: > > Ping^4. Who is collecting target/i386 patches these days ? > > -- PMM > > On Mon, 1 Nov 2021 at 16:18, Peter Maydell wrote: > > > > Ping^3, now 2 months after patch posted and reviewed... > > > > -- PMM > > > > On Mon, 27 Sept 2021 at 11:03, Peter Maydell > > wrote: > > > > > > Ping^2 ! > > > > > > thanks > > > -- PMM > > > > > > On Mon, 13 Sept 2021 at 13:34, Peter Maydell > > > wrote: > > > > > > > > Ping? (this has been reviewed) > > > > > > > > thanks > > > > -- PMM > > > > > > > > On Wed, 1 Sept 2021 at 15:10, Peter Maydell > > > > 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 > > > > > --- > > > > > 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
Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
Ping^4. Who is collecting target/i386 patches these days ? -- PMM On Mon, 1 Nov 2021 at 16:18, Peter Maydell wrote: > > Ping^3, now 2 months after patch posted and reviewed... > > -- PMM > > On Mon, 27 Sept 2021 at 11:03, Peter Maydell wrote: > > > > Ping^2 ! > > > > thanks > > -- PMM > > > > On Mon, 13 Sept 2021 at 13:34, Peter Maydell > > wrote: > > > > > > Ping? (this has been reviewed) > > > > > > thanks > > > -- PMM > > > > > > On Wed, 1 Sept 2021 at 15:10, Peter Maydell > > > 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 > > > > --- > > > > 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
Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
Ping^3, now 2 months after patch posted and reviewed... -- PMM On Mon, 27 Sept 2021 at 11:03, Peter Maydell wrote: > > Ping^2 ! > > thanks > -- PMM > > On Mon, 13 Sept 2021 at 13:34, Peter Maydell wrote: > > > > Ping? (this has been reviewed) > > > > thanks > > -- PMM > > > > On Wed, 1 Sept 2021 at 15:10, Peter Maydell > > 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 > > > --- > > > 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
Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
Ping^2 ! thanks -- PMM On Mon, 13 Sept 2021 at 13:34, Peter Maydell wrote: > > Ping? (this has been reviewed) > > thanks > -- PMM > > On Wed, 1 Sept 2021 at 15:10, Peter Maydell 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 > > --- > > 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
Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
Ping? (this has been reviewed) thanks -- PMM On Wed, 1 Sept 2021 at 15:10, Peter Maydell 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 > --- > 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
Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
On 9/1/21 4:10 PM, Peter Maydell 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 --- v1->v2: use assert() rather than just deleting the if()s Reviewed-by: Richard Henderson r~
[PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
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 --- 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