RE: [PATCH] ioreq: cope with server disappearing while I/O is pending

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Sent: 05 October 2020 15:30
> To: Paul Durrant <paul@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Durrant, Paul <pdurrant@xxxxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Jan 
> Beulich
> <jbeulich@xxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; Wei Liu 
> <wl@xxxxxxx>
> Subject: RE: [EXTERNAL] [PATCH] ioreq: cope with server disappearing while 
> I/O is pending
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 05/10/2020 15:08, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@xxxxxxxxxx>
> >
> > Currently, in the event of an ioreq server being destroyed while I/O is
> > pending in the attached emulator, it is possible that hvm_wait_for_io() will
> > dereference a pointer to a 'struct hvm_ioreq_vcpu' or the ioreq server's
> > shared page after it has been freed.
> 
> It's not legitimate for the shared page to be freed while Xen is still
> using it.
> 
> Furthermore, this patch only covers the most obvious place for the bug
> to manifest.  It doesn't fix them all, as a ioreq server destroy can
> race with an in-progress emulation and still suffer a UAF.
> 
> 
> An extra ref needs holding on the shared page between acquire_resource
> and domain destruction, to cover Xen's use of the page.
> 

Yes, that's true.

> Similarly, I don't think it is legitimate for hvm_ioreq_vcpu to be freed
> while potentially in use.  These need to stick around until domain
> destruction as well, I think.
> 

That would cover the problem with the sv pointer, I guess it would be possible 
to mark the struct stale and then free it when 'pending' transitions to false.

> > This will only occur if the emulator (which is necessarily running in a
> > service domain with some degree of privilege) does not complete pending I/O
> > during tear-down and is not directly exploitable by a guest domain.
> >
> > This patch adds a call to get_pending_vcpu() into the condition of the
> > wait_on_xen_event_channel() macro to verify the continued existence of the
> > ioreq server. Should it disappear, the guest domain will be crashed.
> >
> > NOTE: take the opportunity to modify the text on one gdprintk() for
> >       consistency with others.
> 
> I know this is tangential, but all these gdprintk()'s need to change to
> gprintk()'s, so release builds provide at least some hint as to why the
> domain has been crashed.
> 

Yes, that's also a good point.

I guess this will patch will probably need to become a series.

  Paul

> (And I also really need to finish off my domain_crash() patch to force
> this everywhere.)
> 
> ~Andrew