| Message ID | b9bbc584-db3c-0b03-0314-3dd907f645bc@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/PV: address odd UB in I/O emulation | expand |
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@qq.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > 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
On 08.07.2021 13:15, Andrew Cooper wrote: > 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. And how is a negative displacement a problem? ptrdiff_t is explicitly a signed type. The language (I'm inclined to say "of course") allows for pointer subtraction in both directions, i.e. one isn't required to know which one of the operands is pointing to the lower address. > This is specifically why I did the cast the other way around, so we're > subtracting integers not pointers. Iirc what you did was to cast the result of the pointer arithmetic. The fact that this has silenced the reporting of UB was suspicious in the first place, as I did also point out ... > 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. ... here. > Further to the above, that was also so didn't have an expression of > (ptr) - (unsigned long). We didn't have such an expression - the left side (the function pointer) is already being cast to 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. Converting _from_ signed _to_ unsigned does, but the other way around in only has if the value is representable (i.e. doesn't end up negative). >> 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@qq.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> 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. There's no concept of "const" when the pointed to type is a function type. Or else the compiler would legitimately complain about the loss of const. >> --- 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 per above I don't see why you claim this is the only UB-free form; for now I don't see any UB in the form I've used. Plus your variant again utilizes unsigned -> signed conversion, which I've explained I'd prefer to avoid (but would be willing to give up on, as that's only a secondary goal here, and we surely have very many cases of such throughout the code base). Jan
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@qq.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > 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 > >
On Thu, Jul 08, 2021 at 09:21:26AM +0200, 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@qq.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
--- 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
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@qq.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- 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.