From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: [PATCH 21/22] libxc: range checks in xc_dom_p2m_host and _guest Date: Tue, 11 Jun 2013 17:19:15 +0100 Message-ID: <20130611161915.GB27333@ocelot.phlegethon.org> References: <1370629642-6990-1-git-send-email-ian.jackson@eu.citrix.com> <1370629642-6990-22-git-send-email-ian.jackson@eu.citrix.com> <51B2278C.8030507@citrix.com> <51B31DF3.7090201@citrix.com> <20919.18742.271615.149261@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20919.18742.271615.149261@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Andrew Cooper , "xen-devel@lists.xensource.com" , "mattjd@gmail.com" , "security@xen.org" List-Id: xen-devel@lists.xenproject.org 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.