From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v7][RFC][PATCH 01/13] xen: RMRR fix Date: Thu, 30 Oct 2014 16:21:47 +0800 Message-ID: <5451F51B.6080700@intel.com> References: <1414136077-18599-1-git-send-email-tiejun.chen@intel.com> <544A4B7A0200007800041E0D@mail.emea.novell.com> <544DA74A.4020505@intel.com> <544E214F020000780004253D@mail.emea.novell.com> <544F5570.5050106@intel.com> <544F71270200007800042B51@mail.emea.novell.com> <5450395A.2080703@intel.com> <54505649.1000103@intel.com> <5450B7510200007800042FF1@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5450B7510200007800042FF1@mail.emea.novell.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: Jan Beulich Cc: yang.z.zhang@intel.com, kevin.tian@intel.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 2014/10/29 16:45, Jan Beulich wrote: >>>> On 29.10.14 at 03:51, wrote: >> On 2014/10/29 8:48, Chen, Tiejun wrote: >>> On 2014/10/28 17:34, Jan Beulich wrote: >>>>>>> On 28.10.14 at 09:36, wrote: >>>>> On 2014/10/27 17:41, Jan Beulich wrote: >>>>>>>>> On 27.10.14 at 03:00, wrote: >>>>>>> n 2014/10/24 18:52, Jan Beulich wrote: >>>>>>>>>>> On 24.10.14 at 09:34, wrote: >>>>>>>>> Now in our case we add a rule: >>>>>>>>> - if p2m_access_n is set we also set this mapping. >>>>>>>> >>>>>>>> Does that not conflict with eventual use mem-access makes of this >>>>>>>> type? >>>>> >>>>> Do you mean what will happen after we reset these ranges as >>>>> p2m_access_rw? We already reserve these ranges guest shouldn't access >>>>> these range actually. And a guest still maliciously access them, that >>>>> device may not work well. >>>> >>>> mem-access is functionality used by a control domain, not the domain >>> >>> I really don't know this mechanism so thanks for your good coverage. >>> >>>> itself. You need to make sure that neither your use of p2m_access_n >>>> can confuse the mem-access code, nor that their use can confuse you. >>> >>> Absolutely, but I think I need to know more about mem-access firstly. >>> >> >> I think these reserved device memory shouldn't be pocked since any write >> may affect device. Even, what if a device with RMRR isn't assign current >> domain? And read also should not be allowed since this still may >> introduce some potential unexpected behavior to device. >> >> So if mem_access is trying to access those RMRR range, could we let >> mem_access exit directly with some message? I mean we can check if we're >> accessing those RMRR ranges in case of XENMEM_access_op_set_access. > > Sounds reasonable at first glance. > I think we just guarantee no one set mem_access for those ranges, but its fine to get mem_access: diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c index 6c2724b..4c84f88 100644 --- a/xen/common/mem_access.c +++ b/xen/common/mem_access.c @@ -55,6 +55,31 @@ void mem_access_resume(struct domain *d) } } +/* We can't expose reserved device memory. */ +static int mem_access_check_rdm(struct domain *d, uint64_aligned_t start, + uint32_t nr) +{ + uint32_t i; + uint64_aligned_t gfn; + int rc = 0; + + if ( !is_hardware_domain(d) ) + { + for ( i = 0; i < nr; i++ ) + { + gfn = start + i; + rc = iommu_get_reserved_device_memory(p2m_check_reserved_device_memory, + &gfn); + if ( rc < 0 ) + printk(XENLOG_WARNING + "Domain %d can't check reserved device memory.\n", + d->domain_id); + } + } + + return rc; +} + int mem_access_memop(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg) { @@ -99,6 +124,15 @@ int mem_access_memop(unsigned long cmd, ((mao.pfn + mao.nr - 1) > domain_get_maximum_gpfn(d))) ) break; + rc = mem_access_check_rdm(d, mao.pfn, mao.nr); + if ( rc == 1 ) + { + printk(XENLOG_WARNING + "Domain %d: we shouldn't mem_access reserved device memory.\n", + d->domain_id); + break; + } + rc = p2m_set_mem_access(d, mao.pfn, mao.nr, start_iter, MEMOP_CMD_MASK, mao.access); if ( rc > 0 ) Thanks Tiejun