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

[Juergen Gross]

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