Re: [Xen-devel] [PATCH MM-PART3 v2 06/12] xen/arm: mm: Sanity check any update of Xen page tables

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 12/06/2019 01:10, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> The code handling Xen PT update has quite a few restrictions on what it
>> can do. This is not a bad thing as it keeps the code simple.
>>
>> There are already a few checks scattered in current page table handling.
>> However they are not sufficient as they could still allow to
>> modify/remove entry with contiguous bit set.
>>
>> The checks are divided in two sets:
>>      - per entry check: They are gathered in a new function that will
>>      check whether an update is valid based on the flags passed and the
>>      current value of an entry.
>>      - global check: They are sanity check on xen_pt_update() parameters.
>>
>> Additionally to contiguous check, we also now check that the caller is
>> not trying to modify the memory attributes of an entry.
>>
>> Lastly, it was probably a bit over the top to forbid removing an
>> invalid mapping. This could just be ignored. The new behavior will be
>> helpful in future changes.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>
>> ---
>>      Changes in v2:
>>          - Correctly detect the removal of a page
>>          - Fix ASSERT on flags in the else case
>>          - Add Andrii's reviewed-by
>> ---
>>   xen/arch/arm/mm.c | 115 +++++++++++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 97 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 2192dede55..45a6f9287f 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -50,6 +50,19 @@ struct domain *dom_xen, *dom_io, *dom_cow;
>>   #undef mfn_to_virt
>>   #define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
>>   
>> +#ifdef NDEBUG
>> +static inline void
>> +__attribute__ ((__format__ (__printf__, 1, 2)))
>> +mm_printk(const char *fmt, ...) {}
>> +#else
>> +#define mm_printk(fmt, args...)             \
>> +    do                                      \
>> +    {                                       \
>> +        dprintk(XENLOG_ERR, fmt, ## args);  \
>> +        WARN();                             \
>> +    } while (0);
>> +#endif
>> +
>>   #define DEFINE_PAGE_TABLES(name, nr)                    \
>>   lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
>>   
>> @@ -968,12 +981,74 @@ enum xenmap_operation {
>>       RESERVE
>>   };
>>   
>> +/* Sanity check of the entry */
>> +static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>> +{
>> +    /* Sanity check when modifying a page. */
>> +    if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
>> +    {
> 
> I understand we could skip the valid check on REMOVE, but should we skip
> it on MODIFY too? Is that also going to be helpful in future changes?

Hmmm, I can't exactly remember why I didn't check the valid bit here.

I did it for REMOVE as for the early FDT mapping it is more convenient to remove 
the full possible range over keeping track of the exact start/size.

I would assume the same would hold for MODIFY, but I don't have a concrete 
example here. I am happy to add the valid check and defer the decision to remove 
it if it is deem to be useful in the future.

> 
> 
>> +        /* We don't allow changing memory attributes. */
>> +        if ( entry.pt.ai != PAGE_AI_MASK(flags) )
>> +        {
>> +            mm_printk("Modifying memory attributes is not allowed (0x%x -> 0x%x).\n",
>> +                      entry.pt.ai, PAGE_AI_MASK(flags));
>> +            return false;
>> +        }
>> +
>> +        /* We don't allow modifying entry with contiguous bit set. */
>> +        if ( entry.pt.contig )
>> +        {
>> +            mm_printk("Modifying entry with contiguous bit set is not allowed.\n");
>> +            return false;
>> +        }
>> +    }
>> +    /* Sanity check when inserting a page */
>> +    else if ( flags & _PAGE_PRESENT )
>> +    {
>> +        /* We should be here with a valid MFN. */
>> +        ASSERT(!mfn_eq(mfn, INVALID_MFN));
>> +
>> +        /* We don't allow replacing any valid entry. */
>> +        if ( lpae_is_valid(entry) )
>> +        {
>> +            mm_printk("Changing MFN for a valid entry is not allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n",
>> +                      mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
>> +            return false;
>> +        }
>> +    }
>> +    /* Sanity check when removing a page. */
>> +    else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 )
>> +    {
>> +        /* We should be here with an invalid MFN. */
>> +        ASSERT(mfn_eq(mfn, INVALID_MFN));
>> +
>> +        /* We don't allow removing page with contiguous bit set. */
>> +        if ( entry.pt.contig )
>> +        {
>> +            mm_printk("Removing entry with contiguous bit set is not allowed.\n");
>> +            return false;
>> +        }
>> +    }
>> +    /* Sanity check when populating the page-table. No check so far. */
>> +    else
>> +    {
>> +        ASSERT(flags & _PAGE_POPULATE);
>> +        /* We should be here with an invalid MFN */
>> +        ASSERT(mfn_eq(mfn, INVALID_MFN));
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
>>                                  mfn_t mfn, unsigned int flags)
>>   {
>>       lpae_t pte, *entry;
>>       lpae_t *third = NULL;
>>   
>> +    /* _PAGE_POPULATE and _PAGE_PRESENT should never be set together. */
>> +    ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) != (_PAGE_POPULATE|_PAGE_PRESENT));
> 
> over 80 chars?

It is 87 chars, I was hoping you didn't notice it :). The line splitting result 
to nasty code. Alternatively, I could introduce a define for 
_PAGE_POPULATE|_PAGE_PRESENT, maybe EXCLUSIVE_FLAGS?

Any preference?

> 
> 
>>       entry = &xen_second[second_linear_offset(addr)];
>>       if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
>>       {
>> @@ -989,15 +1064,12 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
>>       third = mfn_to_virt(lpae_get_mfn(*entry));
>>       entry = &third[third_table_offset(addr)];
>>   
>> +    if ( !xen_pt_check_entry(*entry, mfn, flags) )
>> +        return -EINVAL;
>> +
>>       switch ( op ) {
>>           case INSERT:
>>           case RESERVE:
>> -            if ( lpae_is_valid(*entry) )
>> -            {
>> -                printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
>> -                       __func__, addr, mfn_x(mfn));
>> -                return -EINVAL;
>> -            }
>>               if ( op == RESERVE )
>>                   break;
>>               pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
>> @@ -1009,12 +1081,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
>>               break;
>>           case MODIFY:
>>           case REMOVE:
>> -            if ( !lpae_is_valid(*entry) )
>> -            {
>> -                printk("%s: trying to %s a non-existing mapping addr=%lx\n",
>> -                       __func__, op == REMOVE ? "remove" : "modify", addr);
>> -                return -EINVAL;
>> -            }
>>               if ( op == REMOVE )
>>                   pte.bits = 0;
>>               else
>> @@ -1022,12 +1088,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
>>                   pte = *entry;
>>                   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;
>> -                }
>>               }
>>               write_pte(entry, pte);
>>               break;
>> @@ -1049,6 +1109,25 @@ static int xen_pt_update(enum xenmap_operation op,
>>       int rc = 0;
>>       unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
>>   
>> +    /*
>> +     * The hardware was configured to forbid mapping both writeable and
>> +     * executable.
>> +     * When modifying/creating mapping (i.e _PAGE_PRESENT is set),
>> +     * prevent any update if this happen.
>> +     */
>> +    if ( (flags & _PAGE_PRESENT) && !PAGE_RO_MASK(flags) &&
>> +         !PAGE_XN_MASK(flags) )
>> +    {
>> +        mm_printk("Mappings should not be both Writeable and Executable.\n");
>> +        return -EINVAL;
>> +    }
> 
> I am thinking this is serious enough that we might want to always print
> this warning when this error happens. At the same time it is awkward to
> have all the other messages using mm_printk and only this one being
> different. So I'll live it to you, it is also OK at this.

Any error here means the caller didn't do sanity check (if input is external) or 
pass the wrong parameters. I purposefully chose mm_printk over a normal printk 
because this could potentially lead to a DoS if accessible from outside of Xen.

If the error happen, then there are an high chance with DEBUG (or hacking 
mm_printk to be used in non-debug build) will make it appear as well.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel