| Message ID | 20190514123125.29086-7-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Provide a generic function to update Xen PT | expand |
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? > + /* 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? > 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. > + if ( !IS_ALIGNED(virt, PAGE_SIZE) ) > + { > + mm_printk("The virtual address is not aligned to the page-size.\n"); > + return -EINVAL; > + } > + > spin_lock(&xen_pt_lock); > > for( ; addr < addr_end; addr += PAGE_SIZE ) > -- > 2.11.0 >
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,
On Wed, 12 Jun 2019, Julien Grall wrote: > 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. Yes, it would be better > > > > > + /* 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? I don't care so much about 80 chars limit. Anything but another macro :-) > > > 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. OK
Hi, On 12/06/2019 16:54, Stefano Stabellini wrote: > On Wed, 12 Jun 2019, Julien Grall wrote: >> On 12/06/2019 01:10, Stefano Stabellini wrote: >>> On Tue, 14 May 2019, Julien Grall wrote: >>> 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. > > Yes, it would be better I will update it in the next version. [...] >>>> 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? > > I don't care so much about 80 chars limit. > Anything but another macro :-) Ok I will keep the 80 lines then! Cheers,
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) ) + { + /* 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)); + 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; + } + + if ( !IS_ALIGNED(virt, PAGE_SIZE) ) + { + mm_printk("The virtual address is not aligned to the page-size.\n"); + return -EINVAL; + } + spin_lock(&xen_pt_lock); for( ; addr < addr_end; addr += PAGE_SIZE )
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(-)