| Message ID | 1501008940-1755-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > Sent: 25 July 2017 19:56 > To: Xen-devel <xen-devel@lists.xen.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich > <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com> > Subject: [PATCH] x86/hvm: Fix boundary check in hvmemul_insn_fetch() > > c/s 0943a03037 added some extra protection for overflowing the emulation > instruction cache, but Coverity points out that boundary condition is off by > one when memcpy()'ing out of the buffer. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Oops. Yes. Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Paul Durrant <paul.durrant@citrix.com> > --- > xen/arch/x86/hvm/emulate.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 495e312..52bed04 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -958,8 +958,8 @@ int hvmemul_insn_fetch( > * Will we overflow insn_buf[]? This shouldn't be able to happen, > * which means something went wrong with instruction decoding... > */ > - if ( insn_off > sizeof(hvmemul_ctxt->insn_buf) || > - (insn_off + bytes) > sizeof(hvmemul_ctxt->insn_buf) ) > + if ( insn_off >= sizeof(hvmemul_ctxt->insn_buf) || > + (insn_off + bytes) >= sizeof(hvmemul_ctxt->insn_buf) ) > { > ASSERT_UNREACHABLE(); > return X86EMUL_UNHANDLEABLE; > -- > 2.1.4
>>> Andrew Cooper <andrew.cooper3@citrix.com> 07/25/17 8:55 PM >>> >--- a/xen/arch/x86/hvm/emulate.c >+++ b/xen/arch/x86/hvm/emulate.c >@@ -958,8 +958,8 @@ int hvmemul_insn_fetch( >* Will we overflow insn_buf[]? This shouldn't be able to happen, >* which means something went wrong with instruction decoding... >*/ >- if ( insn_off > sizeof(hvmemul_ctxt->insn_buf) || >- (insn_off + bytes) > sizeof(hvmemul_ctxt->insn_buf) ) >+ if ( insn_off >= sizeof(hvmemul_ctxt->insn_buf) || >+ (insn_off + bytes) >= sizeof(hvmemul_ctxt->insn_buf) ) I agree with the change to the first line, but are you sure about the second one? At the example of insn_off == 0, surely bytes == sizeof() is fine? Jan
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 495e312..52bed04 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -958,8 +958,8 @@ int hvmemul_insn_fetch( * Will we overflow insn_buf[]? This shouldn't be able to happen, * which means something went wrong with instruction decoding... */ - if ( insn_off > sizeof(hvmemul_ctxt->insn_buf) || - (insn_off + bytes) > sizeof(hvmemul_ctxt->insn_buf) ) + if ( insn_off >= sizeof(hvmemul_ctxt->insn_buf) || + (insn_off + bytes) >= sizeof(hvmemul_ctxt->insn_buf) ) { ASSERT_UNREACHABLE(); return X86EMUL_UNHANDLEABLE;
c/s 0943a03037 added some extra protection for overflowing the emulation instruction cache, but Coverity points out that boundary condition is off by one when memcpy()'ing out of the buffer. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Paul Durrant <paul.durrant@citrix.com> --- xen/arch/x86/hvm/emulate.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)