Re: [Xen-devel] [PATCH RFC] tools/kdd: avoid adversarial optimisation hazard

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Wei Liu writes ("[PATCH RFC] tools/kdd: avoid adversarial optimisation hazard"):
> There have been two attempts to fix kdd build with gcc 8.1
> (437e00fe and 2de2b10b), but building with gcc 8.1 32 bit non-debug
> build still yields the same error as in 437e00fe.
> 
> Ian wrote about adversarial optimisation in [0], one of the key points
> is that computing an out-of-bounds pointer is UB.
...
> Eliminate that UB by using uintptr_t to avoid the compiler reaching
> the conclusion that offset <= sizeof(ctrl).

Since I wrote my complaint, the code has been rearranged so that it
does not call memcpy if the bounds check fails.  nAt, least what I
wrote earlier,

 |  Therefore  ((uint8_t *)&ctrl.c32) + offset
 |  (which is caclulated unconditionally)
 |  is within the stack object `ctrl'.   

is not true of current staging.

It's still very obscure becaause this test

        if (offset > sizeof ctrl.c32 || offset + len > sizeof ctrl.c32) {

depends critically on the size of offset, etc.

Is it not still possible that this test could be fooled ?  Suppose
offset is 0xffffffff.  Then before the test, offset is 0xfffffd33.

I think offset + len might wrap around.  len looks like it can be
at most 65536-L.  So the biggest offset produces:

  0xfffffd33 + (65536-L) > L

which I think can wrap round unless L > 717.

This kind of reasoning is awful.  The code should be rewritten so that
it is obvious that it won't go wrong.  Typically that means
calculating the maximum value of len from a checked value of offset.

  if ( .... || len > sizeof - offset )

Ian.

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