Ping: [PATCH] x86/PV: address odd UB in I/O emulation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.07.2021 09:21, Jan Beulich wrote:
> Compilers are certainly right in detecting UB here, given that fully
> parenthesized (to express precedence) the original offending expression
> was (((stub_va + p) - ctxt->io_emul_stub) + 5), which in fact exhibits
> two overflows in pointer calculations. We really want to calculate
> (p - ctxt->io_emul_stub) first, which is guaranteed to not overflow.
> 
> The issue was observed with clang 9 on 4.13.
> 
> The oddities are
> - the issue was detected on APPEND_CALL(save_guest_gprs), despite the
>   earlier similar APPEND_CALL(load_guest_gprs),
> - merely casting the original offending expression to long was reported
>   to also help.
> 
> While at it also avoid converting guaranteed (with our current address
> space layout) negative values to unsigned long (which has implementation
> defined behavior): Have stub_va be of pointer type. And since it's on an
> immediately adjacent line, also constify this_stubs.
> 
> Fixes: d89e5e65f305 ("x86/ioemul: Rewrite stub generation to be shadow stack 
> compatible")
> Reported-by: Franklin Shen <2284696125@xxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> I'm not going to insist on the part avoiding implementation defined
> behavior here. If I am to drop that, it is less clear whether
> constifying this_stubs would then still be warranted.

While I did respond to all review comments by Andrew, this has not
lead to forward progress here.

Jan

> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -89,8 +89,8 @@ static io_emul_stub_t *io_emul_stub_setu
>          0xc3,       /* ret       */
>      };
>  
> -    struct stubs *this_stubs = &this_cpu(stubs);
> -    unsigned long stub_va = this_stubs->addr + STUB_BUF_SIZE / 2;
> +    const struct stubs *this_stubs = &this_cpu(stubs);
> +    const void *stub_va = (void *)this_stubs->addr + STUB_BUF_SIZE / 2;
>      unsigned int quirk_bytes = 0;
>      char *p;
>  
> @@ -98,7 +98,7 @@ static io_emul_stub_t *io_emul_stub_setu
>  #define APPEND_BUFF(b) ({ memcpy(p, b, sizeof(b)); p += sizeof(b); })
>  #define APPEND_CALL(f)                                                  \
>      ({                                                                  \
> -        long disp = (long)(f) - (stub_va + p - ctxt->io_emul_stub + 5); \
> +        long disp = (void *)(f) - (stub_va + (p - ctxt->io_emul_stub) + 5); \
>          BUG_ON((int32_t)disp != disp);                                  \
>          *p++ = 0xe8;                                                    \
>          *(int32_t *)p = disp; p += 4;                                   \
> @@ -106,7 +106,7 @@ static io_emul_stub_t *io_emul_stub_setu
>  
>      if ( !ctxt->io_emul_stub )
>          ctxt->io_emul_stub =
> -            map_domain_page(_mfn(this_stubs->mfn)) + (stub_va & ~PAGE_MASK);
> +            map_domain_page(_mfn(this_stubs->mfn)) + PAGE_OFFSET(stub_va);
>  
>      p = ctxt->io_emul_stub;
>  
> @@ -141,7 +141,7 @@ static io_emul_stub_t *io_emul_stub_setu
>      block_speculation(); /* SCSB */
>  
>      /* Handy function-typed pointer to the stub. */
> -    return (void *)stub_va;
> +    return stub_va;
>  
>  #undef APPEND_CALL
>  #undef APPEND_BUFF
> 
>