[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


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

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.