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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
> Of Wei Liu
> Sent: 26 July 2018 13:54
> To: Ian Jackson <Ian.Jackson@xxxxxxxxxx>
> Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Marek Marczykowski
> <marmarek@xxxxxxxxxxxxxxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>
> Subject: 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 don’t think there is one. AFAIK kdd was written using a lot reverse 
engineering of observed behaviour of WinDBG talking over an emulated serial 
line.

  Paul

> 
> >
> > 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel