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 <[email protected]>
> Signed-off-by: Jan Beulich <[email protected]>
> ---
> 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
>
>
