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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/07/2021 08: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.

I agree that avoiding this overflow is an improvement, but as I said in
my original analysis, (f) - (expr) also underflows and results in a
negative displacement.

This is specifically why I did the cast the other way around, so we're
subtracting integers not pointers.

It appears that we don't use -fwrapv so in any case, the only way of
doing this without UB is to use unsigned long's everywhere.

> 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.

Further to the above, that was also so didn't have an expression of
(ptr) - (unsigned long).

>
> While at it also avoid converting guaranteed (with our current address
> space layout) negative values to unsigned long (which has implementation
> defined behavior):

?  Converting between signed and unsigned representations has explicitly
well defined behaviour.

>  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.

You're implicitly casting away const now at the return, which is
something you object to in other peoples patches.

>
> --- 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); \

The only version of this which is UB-free is

long disp = (unsigned long)(f) - (stub_va + (p - ctxt->io_emul_stub) + 5);

As long as (p - ctxt->io_emul_stub) doesn't overflow (which it doesn't),
every other piece of that expression is well defined when stub_va stays
as its original type.

~Andrew