| Message ID | CAJhHMCDke60mQgOza1xgTs9sSTcE0HESG7G=f3bQ5sP+0Baz0A@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23/03/2017 17:50, Pranith Kumar wrote: > On Thu, Mar 23, 2017 at 6:27 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 22/03/2017 21:01, Richard Henderson wrote: >>>> >>>> Ah, OK. Thanks for the explanation. May be we should check the size of >>>> the instruction while decoding the prefixes and error out once we >>>> exceed the limit. We would not generate any IR code. >>> >>> Yes. >>> >>> It would not enforce a true limit of 15 bytes, since you can't know that >>> until you've done the rest of the decode. But you'd be able to say that >>> no more than 14 prefix + 1 opc + 6 modrm+sib+ofs + 4 immediate = 25 >>> bytes is used. >>> >>> Which does fix the bug. >> >> Yeah, that would work for 2.9 if somebody wants to put together a patch. >> Ensuring that all instruction fetching happens before translation side >> effects is a little harder, but perhaps it's also the opportunity to get >> rid of s->rip_offset which is a little ugly. > > How about the following? > > diff --git a/target/i386/translate.c b/target/i386/translate.c > index 72c1b03a2a..67c58b8900 100644 > --- a/target/i386/translate.c > +++ b/target/i386/translate.c > @@ -4418,6 +4418,11 @@ static target_ulong disas_insn(CPUX86State > *env, DisasContext *s, > s->vex_l = 0; > s->vex_v = 0; > next_byte: > + /* The prefixes can atmost be 14 bytes since x86 has an upper > + limit of 15 bytes for the instruction */ > + if (s->pc - pc_start > 14) { > + goto illegal_op; > + } > b = cpu_ldub_code(env, s->pc); > s->pc++; > /* Collect prefixes. */ Please make the comment more verbose, based on Richard's remark. We should apply it to 2.9. Also, QEMU usually formats comments with stars on every line. Paolo
On Thu, Mar 23, 2017 at 1:37 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 23/03/2017 17:50, Pranith Kumar wrote: >> On Thu, Mar 23, 2017 at 6:27 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> >>> On 22/03/2017 21:01, Richard Henderson wrote: >>>>> >>>>> Ah, OK. Thanks for the explanation. May be we should check the size of >>>>> the instruction while decoding the prefixes and error out once we >>>>> exceed the limit. We would not generate any IR code. >>>> >>>> Yes. >>>> >>>> It would not enforce a true limit of 15 bytes, since you can't know that >>>> until you've done the rest of the decode. But you'd be able to say that >>>> no more than 14 prefix + 1 opc + 6 modrm+sib+ofs + 4 immediate = 25 >>>> bytes is used. >>>> >>>> Which does fix the bug. >>> >>> Yeah, that would work for 2.9 if somebody wants to put together a patch. >>> Ensuring that all instruction fetching happens before translation side >>> effects is a little harder, but perhaps it's also the opportunity to get >>> rid of s->rip_offset which is a little ugly. >> >> How about the following? >> >> diff --git a/target/i386/translate.c b/target/i386/translate.c >> index 72c1b03a2a..67c58b8900 100644 >> --- a/target/i386/translate.c >> +++ b/target/i386/translate.c >> @@ -4418,6 +4418,11 @@ static target_ulong disas_insn(CPUX86State >> *env, DisasContext *s, >> s->vex_l = 0; >> s->vex_v = 0; >> next_byte: >> + /* The prefixes can atmost be 14 bytes since x86 has an upper >> + limit of 15 bytes for the instruction */ >> + if (s->pc - pc_start > 14) { >> + goto illegal_op; >> + } >> b = cpu_ldub_code(env, s->pc); >> s->pc++; >> /* Collect prefixes. */ > > Please make the comment more verbose, based on Richard's remark. We > should apply it to 2.9. > > Also, QEMU usually formats comments with stars on every line. OK. I'll send a proper patch with updated comment. Thanks, -- Pranith
diff --git a/target/i386/translate.c b/target/i386/translate.c index 72c1b03a2a..67c58b8900 100644 --- a/target/i386/translate.c +++ b/target/i386/translate.c @@ -4418,6 +4418,11 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s, s->vex_l = 0; s->vex_v = 0; next_byte: + /* The prefixes can atmost be 14 bytes since x86 has an upper + limit of 15 bytes for the instruction */ + if (s->pc - pc_start > 14) { + goto illegal_op; + } b = cpu_ldub_code(env, s->pc); s->pc++;