* [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
@ 2021-09-01 14:10 Peter Maydell
2021-09-03 13:10 ` Richard Henderson
2021-09-13 12:34 ` Peter Maydell
0 siblings, 2 replies; 7+ messages in thread
From: Peter Maydell @ 2021-09-01 14:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
2021-09-01 14:10 [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode Peter Maydell
@ 2021-09-03 13:10 ` Richard Henderson
2021-09-13 12:34 ` Peter Maydell
1 sibling, 0 replies; 7+ messages in thread
From: Richard Henderson @ 2021-09-03 13:10 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: Paolo Bonzini, Eduardo Habkost
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<peter.maydell@linaro.org>
> ---
> v1->v2: use assert() rather than just deleting the if()s
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
2021-09-01 14:10 [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode Peter Maydell
2021-09-03 13:10 ` Richard Henderson
@ 2021-09-13 12:34 ` Peter Maydell
2021-09-27 10:03 ` Peter Maydell
1 sibling, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2021-09-13 12:34 UTC (permalink / raw)
To: QEMU Developers; +Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
2021-09-13 12:34 ` Peter Maydell
@ 2021-09-27 10:03 ` Peter Maydell
2021-11-01 16:18 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2021-09-27 10:03 UTC (permalink / raw)
To: QEMU Developers; +Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
2021-09-27 10:03 ` Peter Maydell
@ 2021-11-01 16:18 ` Peter Maydell
2021-11-15 14:38 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2021-11-01 16:18 UTC (permalink / raw)
To: QEMU Developers; +Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
2021-11-01 16:18 ` Peter Maydell
@ 2021-11-15 14:38 ` Peter Maydell
2021-12-09 20:01 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2021-11-15 14:38 UTC (permalink / raw)
To: QEMU Developers; +Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode
2021-11-15 14:38 ` Peter Maydell
@ 2021-12-09 20:01 ` Peter Maydell
0 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2021-12-09 20:01 UTC (permalink / raw)
To: QEMU Developers; +Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-12-09 20:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-01 14:10 [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode Peter Maydell
2021-09-03 13:10 ` Richard Henderson
2021-09-13 12:34 ` Peter Maydell
2021-09-27 10:03 ` Peter Maydell
2021-11-01 16:18 ` Peter Maydell
2021-11-15 14:38 ` Peter Maydell
2021-12-09 20:01 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.