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

[Jan Beulich]

>>> On 21.06.17 at 17:12,  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 

Reviewed-by: Jan Beulich 



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

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

[Paul Durrant]

> -Original Message-
> From: Andrew Cooper
> Sent: 21 June 2017 17:15
> To: Paul Durrant ; Xen-devel  de...@lists.xen.org>
> Cc: Jan Beulich 
> 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.coop...@citrix.com]
> >> Sent: 21 June 2017 16:12
> >> To: Xen-devel 
> >> Cc: Andrew Cooper ; Jan Beulich
> >> ; Paul Durrant 
> >> 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 
> >> ---
> >> CC: Jan Beulich 
> >> CC: Paul Durrant 
> >>
> >> 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 

> 
> ~Andrew

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

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

[Andrew Cooper]

On 21/06/17 17:04, Paul Durrant wrote:
>> -Original Message-
>> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
>> Sent: 21 June 2017 16:12
>> To: Xen-devel 
>> Cc: Andrew Cooper ; Jan Beulich
>> ; Paul Durrant 
>> 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 
>> ---
>> CC: Jan Beulich 
>> CC: Paul Durrant 
>>
>> 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

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

[Paul Durrant]

> -Original Message-
> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> Sent: 21 June 2017 16:12
> To: Xen-devel 
> Cc: Andrew Cooper ; Jan Beulich
> ; Paul Durrant 
> 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 
> ---
> CC: Jan Beulich 
> CC: Paul Durrant 
> 
> 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(_ctxt->insn_buf[insn_off], p_data, bytes);
>  hvmemul_ctxt->insn_buf_bytes = insn_off + bytes;
>  }
> --
> 2.1.4


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

[Xen-devel] [PATCH 1/6] x86/hvm: Fixes to hvmemul_insn_fetch()

[Andrew Cooper]

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 
---
CC: Jan Beulich 
CC: Paul Durrant 

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;
 
 /*
  * 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(_ctxt->insn_buf[insn_off], p_data, bytes);
 hvmemul_ctxt->insn_buf_bytes = insn_off + bytes;
 }
-- 
2.1.4


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