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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jul 26, 2018 at 01:37:45PM +0100, Ian Jackson wrote:
> 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.

I don't think that was true when you wrote your complaint either. That's
why I got confused and wrote  "I don't follow the calculated
unconditionally bit" in this patch.

> 
> 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 also had this question.

I suspect the address, from which offset is derived, is bounded. But I
haven't found the spec for KD.

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

Yes, I think getting offset checked is rather helpful. I didn't do that
because I didn't know what range it was supposed to be in.

Wei.

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

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