Re: [Xen-devel] [PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-07-20 at 18:00 +0100, Daniel De Graaf wrote:
> On 07/20/2012 12:30 PM, Ian Campbell wrote:
> > On Fri, 2012-07-20 at 17:06 +0100, Ian Jackson wrote:
> >> Ian Campbell writes ("[PATCH] libxc: restore: bounds check for 
> >> start_info.{store_mfn, console.domU.mfn}"):
> >>> libxc: restore: bounds check for start_info.{store_mfn,console.domU.mfn}
> >>>
> >>> These fields are canonicalised by the guest on suspend and therefore must 
> >>> be
> >>> valid pfns during restore.
> >>>
> >>> Reported-by: Jonathan Ludlam <Jonathan.Ludlam@xxxxxxxxxxxxx>
> >>> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >>
> >> Does this mean that a malicious restore file can take over the
> >> toolstack ?
> > 
> > Good question, I should have considered this before posting.
> > 
> > The value in question is used as an offset into the p2m. So this allows
> > the attacker to read off the end of that array, potentially reading some
> > other word and storing it in either *store_mfn or *console_mfn (or
> > both). Lets assume that the attacker is clever and can control some
> > value which can be seen in this way (perhaps the tools have a guest page
> > mapped which they control).
> > 
> > The values are written to the attacker's guest's start info (harmful
> > only to themselves, I think) and used to seed a grant table entry.
> > Seeding the gnttab would allow the attacker to potentially grant access
> > to some other domain to one of the attacker's domain's own pages, which
> > again seems harmless enough. You cannot grant a page you do not own so
> > there is no way to leak information that way.
> > 
> > The *foo_mfn pointers are arguments to the xc_domain_restore function
> > and are then used by the toolstack to write the mfns to xenstore and for
> > xs_domain_introduce (I can't see any other use in libxl/xl).
> > 
> > I believe both xenconsoled and xenstored will default to using the grant
> > table entries seeded above these days, which will prevent them from
> > inadvertently mapping a page other than that owned by the attacher's
> > guest.
> 
> Actually, it's just xenstored that was changed (oxenstored was not). I
> have a patch to do the same for xenconsoled saved for when 4.3 opens, but
> it was regarded as too late for 4.2 last time I mentioned it.

That rings a bell. Sorry this freeze has been dragging on for so
long :-/

> > Some versions of those daemons use the mmap foreign privileged
> > interface. I suppose this could be used to trick xenconsoled into
> > treating an arbitrary page as the guests console or to trick xenstored
> > into treating an arbitrary page as a xenstore ring. I'm not sure if that
> > is dangerous or not.
> 
> The map_foreign_range call does include a domain ID all the way up to the
> hypervisor, which prevents the daemons from mapping pages that the target
> domain in question isn't able to map on its own.

Thanks for pointing that out, I'd forgotten about that param.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel