Re: [PATCH 1/6] x86/hvm: Fixes to hvmemul_insn_fetch()

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel