| Message ID | 20170814142418.13267-27-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: Memory subsystem clean-up | expand |
Hi, 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@arm.com> > --- > 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 ) > break; > 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 ) > + { > + 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. Cheers, Andre. > + } > pte.pt.table = 1; > write_pte(entry, pte); > break; >
On 08/23/2017 03:09 PM, Andre Przywara wrote: > Hi, Hi, > > 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@arm.com> >> --- >> 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 ) >> break; >> 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. Cheers,
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 ) break; 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 ) + { + printk("%s: Incorrect combination for addr=%lx\n", + __func__, addr); + return -EINVAL; + } pte.pt.table = 1; write_pte(entry, pte); break;
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@arm.com> --- xen/arch/arm/mm.c | 8 ++++++++ 1 file changed, 8 insertions(+)