[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decod
From: |
Peter Maydell |
Subject: |
Re: [PATCH v2] target/i386: Use assert() to sanity-check b1 in SSE decode |
Date: |
Mon, 13 Sep 2021 13:34:10 +0100 |
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