Re: [Xen-devel] [PATCH 21/22] libxc: range checks in xc_dom_p2m_host and _guest

[Tim Deegan <tim@xxxxxxx>]

At 16:58 +0100 on 11 Jun (1370969926), Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH 21/22] libxc: range checks in 
> xc_dom_p2m_host and _guest"):
> > On 07/06/2013 19:33, Andrew Cooper wrote:
> > > On 07/06/13 19:27, Ian Jackson wrote:
> > >> v6: Check for underflow too (thanks to Andrew Cooper).
> ...
> > >> diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
> > >> index 567913f..2e33ee7 100644
> > >> --- a/tools/libxc/xc_dom.h
> > >> +++ b/tools/libxc/xc_dom.h
> > >> @@ -341,6 +341,8 @@ static inline xen_pfn_t xc_dom_p2m_host(struct 
> > >> xc_dom_image *dom, xen_pfn_t pfn)
> > >>  {
> > >>      if (dom->shadow_enabled)
> > >>          return pfn;
> > 
> > The above should probably be
> > 
> > if (dom->shadow_enabled)
> >     return pfn < dom->total_pages ? pfn : INVALID_MFN;
> > 
> > So the dom->shadow_enable case also gets upper range checking.
> 
> Are you sure this shouldn't involve rambase_pfn, as the next test
> does ?  Here:
> 
> > >> +    if (pfn < dom->rambase_pfn || pfn >= dom->rambase_pfn + 
> > >> dom->total_pages)
> > >> +        return INVALID_MFN;
> 
> If it should then the right fix would be to move the check to before
> the shadow_enabled test.  (In both xc_dom_vaddr_to_ptr and
> xc_dom_p2m_guest.)
> 
> I'm not very familiar with the semantics of these functions.  I've
> CC'd Tim Deegan who can maybe help advise...

With the caveat that I'm not familiar with the PV builder code I'd say
the shadow case is fine without extra checks.  If the guest is really
paging_mode_translate then Xen will reject any invalid gfns when the
caller tries to map them.

Cheers,

Tim.

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