Re: [Xen-devel] [PATCH 1/2] x86: consider effective protection attributes in W+X check

[Juergen Gross <jgross@xxxxxxxx>]

On 12/12/17 11:31, Jan Beulich wrote:
> Using just the leaf page table entry flags would cause a false warning
> in case _PAGE_RW is clear or _PAGE_NX is set in a higher level entry.
> Hand through both the current entry's flags as well as the accumulated
> effective value (the latter as pgprotval_t instead of pgprot_t, as it's
> not an actual entry's value).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>  arch/x86/mm/dump_pagetables.c |   92 
> ++++++++++++++++++++++++++----------------
>  1 file changed, 57 insertions(+), 35 deletions(-)
> 
> --- 4.15-rc3/arch/x86/mm/dump_pagetables.c
> +++ 4.15-rc3-x86-dumppgt-effective-prot/arch/x86/mm/dump_pagetables.c
> @@ -29,6 +29,7 @@
>  struct pg_state {
>       int level;
>       pgprot_t current_prot;
> +     pgprotval_t effective_prot;
>       unsigned long start_address;
>       unsigned long current_address;
>       const struct addr_marker *marker;
> @@ -202,9 +203,9 @@ static unsigned long normalize_addr(unsi
>   * print what we collected so far.
>   */
>  static void note_page(struct seq_file *m, struct pg_state *st,
> -                   pgprot_t new_prot, int level)
> +                   pgprot_t new_prot, pgprotval_t new_eff, int level)
>  {
> -     pgprotval_t prot, cur;
> +     pgprotval_t prot, cur, eff;
>       static const char units[] = "BKMGTPE";
>  
>       /*
> @@ -214,23 +215,24 @@ static void note_page(struct seq_file *m
>        */
>       prot = pgprot_val(new_prot);
>       cur = pgprot_val(st->current_prot);
> +     eff = st->effective_prot;
>  
>       if (!st->level) {
>               /* First entry */
>               st->current_prot = new_prot;
> +             st->effective_prot = new_eff;
>               st->level = level;
>               st->marker = address_markers;
>               st->lines = 0;
>               pt_dump_seq_printf(m, st->to_dmesg, "---[ %s ]---\n",
>                                  st->marker->name);
> -     } else if (prot != cur || level != st->level ||
> +     } else if (prot != cur || new_eff != eff || level != st->level ||
>                  st->current_address >= st->marker[1].start_address) {
>               const char *unit = units;
>               unsigned long delta;
>               int width = sizeof(unsigned long) * 2;
> -             pgprotval_t pr = pgprot_val(st->current_prot);
>  
> -             if (st->check_wx && (pr & _PAGE_RW) && !(pr & _PAGE_NX)) {
> +             if (st->check_wx && (eff & _PAGE_RW) && !(eff & _PAGE_NX)) {
>                       WARN_ONCE(1,
>                                 "x86/mm: Found insecure W+X mapping at 
> address %p/%pS\n",
>                                 (void *)st->start_address,
> @@ -284,21 +286,30 @@ static void note_page(struct seq_file *m
>  
>               st->start_address = st->current_address;
>               st->current_prot = new_prot;
> +             st->effective_prot = new_eff;
>               st->level = level;
>       }
>  }
>  
> -static void walk_pte_level(struct seq_file *m, struct pg_state *st, pmd_t 
> addr, unsigned long P)
> +static inline pgprotval_t effective_prot(pgprotval_t prot1, pgprotval_t 
> prot2)
> +{
> +     return (prot1 & prot2 & (_PAGE_USER | _PAGE_RW)) |
> +            ((prot1 | prot2) & _PAGE_NX);
> +}
> +
> +static void walk_pte_level(struct seq_file *m, struct pg_state *st, pmd_t 
> addr,
> +                        pgprotval_t eff_in, unsigned long P)
>  {
>       int i;
>       pte_t *start;
> -     pgprotval_t prot;
> +     pgprotval_t prot, eff;
>  
>       start = (pte_t *)pmd_page_vaddr(addr);
>       for (i = 0; i < PTRS_PER_PTE; i++) {
>               prot = pte_flags(*start);
> +             eff = effective_prot(eff_in, prot);
>               st->current_address = normalize_addr(P + i * PTE_LEVEL_MULT);
> -             note_page(m, st, __pgprot(prot), 5);
> +             note_page(m, st, __pgprot(prot), eff, 5);
>               start++;
>       }
>  }
> @@ -335,42 +346,45 @@ static inline bool kasan_page_table(stru
>  
>  #if PTRS_PER_PMD > 1
>  
> -static void walk_pmd_level(struct seq_file *m, struct pg_state *st, pud_t 
> addr, unsigned long P)
> +static void walk_pmd_level(struct seq_file *m, struct pg_state *st, pud_t 
> addr,
> +                        pgprotval_t eff_in, unsigned long P)
>  {
>       int i;
>       pmd_t *start, *pmd_start;
> -     pgprotval_t prot;
> +     pgprotval_t prot, eff;
>  
>       pmd_start = start = (pmd_t *)pud_page_vaddr(addr);
>       for (i = 0; i < PTRS_PER_PMD; i++) {
>               st->current_address = normalize_addr(P + i * PMD_LEVEL_MULT);
>               if (!pmd_none(*start)) {
> +                     prot = pmd_flags(*start);
> +                     eff = effective_prot(eff_in, prot);
>                       if (pmd_large(*start) || !pmd_present(*start)) {
> -                             prot = pmd_flags(*start);
> -                             note_page(m, st, __pgprot(prot), 4);
> +                             note_page(m, st, __pgprot(prot), eff, 4);
>                       } else if (!kasan_page_table(m, st, pmd_start)) {
> -                             walk_pte_level(m, st, *start,
> +                             walk_pte_level(m, st, *start, eff,
>                                              P + i * PMD_LEVEL_MULT);
>                       }

You can drop the braces for both cases. Applies to similar
constructs below, too.

With that fixed you can add my:

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>


Juergen

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