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

Message ID 20170814142418.13267-27-julien.grall@arm.com
State New
Headers show
Series
  • xen/arm: Memory subsystem clean-up
Related show

Commit Message

Julien Grall Aug. 14, 2017, 2:24 p.m.
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(+)

Comments

Andre Przywara Aug. 23, 2017, 2:09 p.m. | #1
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;
>
Julien Grall Aug. 23, 2017, 2:36 p.m. | #2
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,

Patch

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;