[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 26/27] xen/arm: mm: Handling permission flags when adding a new mapping

On 08/23/2017 03:09 PM, Andre Przywara wrote:


On 14/08/17 15:24, Julien Grall wrote:
Currently, all the new mappings will be read-write non-executable. Allow the
caller to use other permissions.

Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
  xen/arch/arm/mm.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index cd7bcf7aca..fe0646002e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1022,6 +1022,14 @@ static int create_xen_entries(enum xenmap_operation op,
                  if ( op == RESERVE )
                  pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
+                pte.pt.ro = PAGE_RO_MASK(flags);
+                pte.pt.xn = PAGE_XN_MASK(flags);
+                if (  !pte.pt.ro && !pte.pt.xn )

I noticed I introduced a double-space here. I will fix.

+                {
+                    printk("%s: Incorrect combination for addr=%lx\n",
+                           __func__, addr);
+                    return -EINVAL;

I don't think this should be a handled runtime error, but rather a
BUG_ON() or an ASSERT(). > I chased down the call chain for all 
create_xen_entries() invocations,
and they all stem from some constant (combination of) hard coded flags.
So ending up with an invalid combination here is clearly a bug in the
code and should be treated as such.

Well, you could potentially call with your own flags. I don't see anything to restrict that and might be used for instance to setup early page table.

If we trust the caller will set the right permission, then a BUG_ON() would be fine here. If not, we should definitely return an error for at least non-debug build as the abort later on would be difficult to hunt down.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.