| Message ID | 1412287806-16016-4-git-send-email-namit@cs.technion.ac.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-10-03 01:10+0300, Nadav Amit: > Once an instruction crosses a page boundary, the size read from the second page > disregards the common case that part of the operand resides on the first page. > As a result, fetch of long insturctions may fail, and thereby cause the > decoding to fail as well. > > Signed-off-by: Nadav Amit <namit@cs.technion.ac.il> > --- Good catch, was it thanks to an exhaustive test-suite? Reviewed-by: Radim Kr?má? <rkrcmar@redhat.com> > arch/x86/kvm/emulate.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index 694dfa7..b2e8aba 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -751,8 +751,10 @@ static int __do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt, int op_size) > static __always_inline int do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt, > unsigned size) > { > - if (unlikely(ctxt->fetch.end - ctxt->fetch.ptr < size)) > - return __do_insn_fetch_bytes(ctxt, size); > + unsigned done_size = ctxt->fetch.end - ctxt->fetch.ptr; > + > + if (unlikely(done_size < size)) > + return __do_insn_fetch_bytes(ctxt, size - done_size); > else > return X86EMUL_CONTINUE; > } > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 6, 2014, at 11:50 PM, Radim Kr?má? <rkrcmar@redhat.com> wrote: > 2014-10-03 01:10+0300, Nadav Amit: >> Once an instruction crosses a page boundary, the size read from the second page >> disregards the common case that part of the operand resides on the first page. >> As a result, fetch of long insturctions may fail, and thereby cause the >> decoding to fail as well. >> >> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il> >> --- > > Good catch, was it thanks to an exhaustive test-suite? > > Reviewed-by: Radim Kr?má? <rkrcmar@redhat.com> It was catcher in a test-environment. However, I keep wondering how it did not happen in real guest OS. I think it is due to pure luck, so I recommend to put it in -stable. Nadav-- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 07/10/2014 11:15, Nadav Amit ha scritto: > > On Oct 6, 2014, at 11:50 PM, Radim Kr?má? <rkrcmar@redhat.com> wrote: > >> 2014-10-03 01:10+0300, Nadav Amit: >>> Once an instruction crosses a page boundary, the size read from the second page >>> disregards the common case that part of the operand resides on the first page. >>> As a result, fetch of long insturctions may fail, and thereby cause the >>> decoding to fail as well. >>> >>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il> >>> --- >> >> Good catch, was it thanks to an exhaustive test-suite? >> >> Reviewed-by: Radim Kr?má? <rkrcmar@redhat.com> > > It was catcher in a test-environment. However, I keep wondering how it did not happen in real guest OS. > I think it is due to pure luck, so I recommend to put it in -stable. The shorter the immediate, the more you need an unlucky alignment for this to happen. For example, say you have 10 byte instruction with 2 opcode bytes and one qword immediate. 1) Instruction at 0x1ffd, __do_insn_fetch_bytes is requested 8 bytes instead of 7, but it accepts up to 15 - 3 = 12 bytes and everything works. 2) Instruction at 0x1ff7, __do_insn_fetch_bytes is requested 8 bytes instead of 1. It accepts up to 15 - 9 = 6 bytes and fails. Most emulated instructions have a 4-byte immediate or no immediate at all. Fixes: 5cfc7e0f5e5e1adf998df94f8e36edaf5d30d38e Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 694dfa7..b2e8aba 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -751,8 +751,10 @@ static int __do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt, int op_size) static __always_inline int do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt, unsigned size) { - if (unlikely(ctxt->fetch.end - ctxt->fetch.ptr < size)) - return __do_insn_fetch_bytes(ctxt, size); + unsigned done_size = ctxt->fetch.end - ctxt->fetch.ptr; + + if (unlikely(done_size < size)) + return __do_insn_fetch_bytes(ctxt, size - done_size); else return X86EMUL_CONTINUE; }
Once an instruction crosses a page boundary, the size read from the second page disregards the common case that part of the operand resides on the first page. As a result, fetch of long insturctions may fail, and thereby cause the decoding to fail as well. Signed-off-by: Nadav Amit <namit@cs.technion.ac.il> --- arch/x86/kvm/emulate.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)