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

From: Juergen Gross
Date: Thu Dec 14 2017 - 09:04:18 EST


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