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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

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