| Message ID | 1498057952-13556-2-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: 21 June 2017 16:12 > 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 1/6] x86/hvm: Fixes to hvmemul_insn_fetch() > > Force insn_off to a single byte, as offset can wrap around or truncate with > respect to sh_ctxt->insn_buf_eip under a number of normal circumstances. > > Furthermore, don't use an ASSERT() for bounds checking the write into > hvmemul_ctxt->insn_buf[]. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Paul Durrant <paul.durrant@citrix.com> > > For anyone wondering, this is way to explot XSA-186 > --- > xen/arch/x86/hvm/emulate.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 11e4aba..495e312 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -939,7 +939,8 @@ int hvmemul_insn_fetch( > { > struct hvm_emulate_ctxt *hvmemul_ctxt = > container_of(ctxt, struct hvm_emulate_ctxt, ctxt); > - unsigned int insn_off = offset - hvmemul_ctxt->insn_buf_eip; > + /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */ > + uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip; Why the change to a uint8_t? Paul > > /* > * Fall back if requested bytes are not in the prefetch cache. > @@ -953,7 +954,17 @@ int hvmemul_insn_fetch( > > if ( rc == X86EMUL_OKAY && bytes ) > { > - ASSERT(insn_off + bytes <= sizeof(hvmemul_ctxt->insn_buf)); > + /* > + * 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) ) > + { > + ASSERT_UNREACHABLE(); > + return X86EMUL_UNHANDLEABLE; > + } > + > memcpy(&hvmemul_ctxt->insn_buf[insn_off], p_data, bytes); > hvmemul_ctxt->insn_buf_bytes = insn_off + bytes; > } > -- > 2.1.4
On 21/06/17 17:04, Paul Durrant wrote: >> -----Original Message----- >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] >> Sent: 21 June 2017 16:12 >> 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 1/6] x86/hvm: Fixes to hvmemul_insn_fetch() >> >> Force insn_off to a single byte, as offset can wrap around or truncate with >> respect to sh_ctxt->insn_buf_eip under a number of normal circumstances. >> >> Furthermore, don't use an ASSERT() for bounds checking the write into >> hvmemul_ctxt->insn_buf[]. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Paul Durrant <paul.durrant@citrix.com> >> >> For anyone wondering, this is way to explot XSA-186 >> --- >> xen/arch/x86/hvm/emulate.c | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c >> index 11e4aba..495e312 100644 >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -939,7 +939,8 @@ int hvmemul_insn_fetch( >> { >> struct hvm_emulate_ctxt *hvmemul_ctxt = >> container_of(ctxt, struct hvm_emulate_ctxt, ctxt); >> - unsigned int insn_off = offset - hvmemul_ctxt->insn_buf_eip; >> + /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */ >> + uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip; > Why the change to a uint8_t? XSA-186 caused problems because offset was truncated at 16 bits, but all calculations here are performed at 64 bits wide, then truncated down to 32bits wide. As a result, insn_off could become massively positive. insn_off needs to be less wide than the minimum truncation width of incoming parameters for it to work correctly. Code hitting the emulator can legitimately cause offset to truncate at 32bits WRT EIP, and the only reason we aren't still vulnerable is because insn_off is unsigned int. If it were unsigned long, we'd have another privilege escalation vulnerability. ~Andrew
> -----Original Message----- > From: Andrew Cooper > Sent: 21 June 2017 17:15 > To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen- > devel@lists.xen.org> > Cc: Jan Beulich <JBeulich@suse.com> > Subject: Re: [PATCH 1/6] x86/hvm: Fixes to hvmemul_insn_fetch() > > On 21/06/17 17:04, Paul Durrant wrote: > >> -----Original Message----- > >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > >> Sent: 21 June 2017 16:12 > >> 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 1/6] x86/hvm: Fixes to hvmemul_insn_fetch() > >> > >> Force insn_off to a single byte, as offset can wrap around or truncate with > >> respect to sh_ctxt->insn_buf_eip under a number of normal > circumstances. > >> > >> Furthermore, don't use an ASSERT() for bounds checking the write into > >> hvmemul_ctxt->insn_buf[]. > >> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> --- > >> CC: Jan Beulich <JBeulich@suse.com> > >> CC: Paul Durrant <paul.durrant@citrix.com> > >> > >> For anyone wondering, this is way to explot XSA-186 > >> --- > >> xen/arch/x86/hvm/emulate.c | 15 +++++++++++++-- > >> 1 file changed, 13 insertions(+), 2 deletions(-) > >> > >> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > >> index 11e4aba..495e312 100644 > >> --- a/xen/arch/x86/hvm/emulate.c > >> +++ b/xen/arch/x86/hvm/emulate.c > >> @@ -939,7 +939,8 @@ int hvmemul_insn_fetch( > >> { > >> struct hvm_emulate_ctxt *hvmemul_ctxt = > >> container_of(ctxt, struct hvm_emulate_ctxt, ctxt); > >> - unsigned int insn_off = offset - hvmemul_ctxt->insn_buf_eip; > >> + /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */ > >> + uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip; > > Why the change to a uint8_t? > > XSA-186 caused problems because offset was truncated at 16 bits, but all > calculations here are performed at 64 bits wide, then truncated down to > 32bits wide. As a result, insn_off could become massively positive. > > insn_off needs to be less wide than the minimum truncation width of > incoming parameters for it to work correctly. > > Code hitting the emulator can legitimately cause offset to truncate at > 32bits WRT EIP, and the only reason we aren't still vulnerable is > because insn_off is unsigned int. If it were unsigned long, we'd have > another privilege escalation vulnerability. Ok. Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > > ~Andrew
>>> On 21.06.17 at 17:12, <andrew.cooper3@citrix.com> wrote: > Force insn_off to a single byte, as offset can wrap around or truncate with > respect to sh_ctxt->insn_buf_eip under a number of normal circumstances. > > Furthermore, don't use an ASSERT() for bounds checking the write into > hvmemul_ctxt->insn_buf[]. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 11e4aba..495e312 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -939,7 +939,8 @@ int hvmemul_insn_fetch( { struct hvm_emulate_ctxt *hvmemul_ctxt = container_of(ctxt, struct hvm_emulate_ctxt, ctxt); - unsigned int insn_off = offset - hvmemul_ctxt->insn_buf_eip; + /* Careful, as offset can wrap or truncate WRT insn_buf_eip. */ + uint8_t insn_off = offset - hvmemul_ctxt->insn_buf_eip; /* * Fall back if requested bytes are not in the prefetch cache. @@ -953,7 +954,17 @@ int hvmemul_insn_fetch( if ( rc == X86EMUL_OKAY && bytes ) { - ASSERT(insn_off + bytes <= sizeof(hvmemul_ctxt->insn_buf)); + /* + * 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) ) + { + ASSERT_UNREACHABLE(); + return X86EMUL_UNHANDLEABLE; + } + memcpy(&hvmemul_ctxt->insn_buf[insn_off], p_data, bytes); hvmemul_ctxt->insn_buf_bytes = insn_off + bytes; }
Force insn_off to a single byte, as offset can wrap around or truncate with respect to sh_ctxt->insn_buf_eip under a number of normal circumstances. Furthermore, don't use an ASSERT() for bounds checking the write into hvmemul_ctxt->insn_buf[]. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Paul Durrant <paul.durrant@citrix.com> For anyone wondering, this is way to explot XSA-186 --- xen/arch/x86/hvm/emulate.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)