[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH MM-PART3 v2 06/12] xen/arm: mm: Sanity check any update of Xen page tables
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@xxxxxxx>
Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
