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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 21 June 2017 16:12
> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
> <JBeulich@xxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> 
> 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


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