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

[Julien Grall]

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,

--
Julien Grall

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

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

[Stefano Stabellini]

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 
> > > Reviewed-by: Andrii Anisov 
> > > 
> > > ---
> > >  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));
> > > +
> > > 

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

[Julien Grall]

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 
Reviewed-by: Andrii Anisov 

---
 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;
  
+/* 

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

[Stefano Stabellini]

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 
> Reviewed-by: Andrii Anisov 
> 
> ---
> 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 = _second[second_linear_offset(addr)];
>  if ( 

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

[Julien Grall]

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 
Reviewed-by: Andrii Anisov 

---
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) )
+{
+/* 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 = _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_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