Re: [v7][RFC][PATCH 01/13] xen: RMRR fix

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/10/29 16:45, Jan Beulich wrote:
>>>> On 29.10.14 at 03:51, <tiejun.chen@intel.com> 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, <tiejun.chen@intel.com> wrote:
>>>>> On 2014/10/27 17:41, Jan Beulich wrote:
>>>>>>>>> On 27.10.14 at 03:00, <tiejun.chen@intel.com> wrote:
>>>>>>> n 2014/10/24 18:52, Jan Beulich wrote:
>>>>>>>>>>> On 24.10.14 at 09:34, <tiejun.chen@intel.com> 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