Re: [Xen-devel] [PATCH] tools/kdd: silence gcc 8 warning a different way

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Wei Liu writes ("Re: [PATCH] tools/kdd: silence gcc 8 warning a different way"):
> On Thu, Apr 12, 2018 at 06:04:49AM -0600, Jan Beulich wrote:
> > Older gcc doesn't like "#pragma GCC diagnostic" inside functions.

Surely this commit message should refer to the previous commit ?


I reviewed the previous code to check that the validation was correct:

   if (offset > sizeof ctrl.c32 || offset + len > sizeof ctrl.c32) {
     len = 0;
   } else {
     memcpy(buf, ((uint8_t *)&ctrl.c32) + offset, len);
   }

I see two problems:

1. Adversarial optimissation hazard:

   The compiler may reason as follows:
   It is not legal to compute an out-of-bounds pointer.
   Doing so is UB.
   Therefore  ((uint8_t *)&ctrl.c32) + offset
   (which is caclulated unconditionally)
   is within the stack object `ctrl'.
   Therefore offset <= sizeof(ctrl).

1a. The compiler can continue to reason:
   Suppose offset > sizeof(ctrl.c32) but offset + len <=
   sizeof(ctrl.c32).  Because len is limited to 32-bit
   that can only happen if offset is large enough to
   wrap when len is added.  But I know that offset <= 216.
   So this situation cannot exist.
   Therefore I can remove the check for offset and
   rely only on the check for offset + len.

1b. The compiler can continue to reason:
   So offset <= 216.  I can therefore check that
   offset <= sizeof(ctrl.c32) using an instruction sequence
   that only looks at the bottom byte of offset (which on
   some architectures might be faster).

1c. If sizeof(ctrl.c32) ever becomes the same as sizeof(ctrl),
   the compiler can remove both checks entirely.

2. Style problem:

   Suppose  len = (uint64_t)-1   offset = 1
   The check passes, but the memcpy gets len = bazillion-1.
   In fact, len is a uint32_t so this is not possible but it is
   not possible to review these lines in isolation and see that
   they are correct.  A future programmer might increase the size
   of len, introducing a bug.  You should compile-time assert
   that len is 32-bit, somehow, right next to that check, IMO.

> > +            /* Suppress bogus gcc 8 "out of bounds" warning. */
> > +            const uint8_t *src;
> > +            asm ("" : "=g" (src) : "0" ((uint8_t *)&ctrl.c32 + offset));
> > +            memcpy(buf, src, len);
> 
> The code looks correct to me:

IMO the new code is very hard to follow.  Are we really expecting
every reader of this to know w3hat the asm does ?

At the very least it should be accompanied with an explanation of what
the asm does, for the benefit of readers who don't speak asm.

I think that rather than introducing an asm, we should disable -Werror
for known-buggy compilers.  But it is possible that fixing the
problems I identify above will make the warning go away anyway.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel