Re: [PATCH 1/2] x86: mm: ptdump: Calculate effective permissions correctly

From: Qian Cai <cai@lca.pw>
To: Steven Price <steven.price@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Andy Lutomirski <luto@kernel.org>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	x86@kernel.org, Jan Beulich <jbeulich@suse.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 1/2] x86: mm: ptdump: Calculate effective permissions correctly
Date: Fri, 22 May 2020 14:07:41 -0400	[thread overview]
Message-ID: <20200522180741.GB1337@Qians-MacBook-Air.local> (raw)
In-Reply-To: <20200521152308.33096-2-steven.price@arm.com>

On Thu, May 21, 2020 at 04:23:07PM +0100, Steven Price wrote:
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -249,10 +249,22 @@ static void note_wx(struct pg_state *st, unsigned long addr)
> @@ -270,16 +282,10 @@ static void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
>  	struct seq_file *m = st->seq;
>  
>  	new_prot = val & PTE_FLAGS_MASK;
> +	new_eff = st->prot_levels[level];

This will trigger,

.config (if ever matters):
https://raw.githubusercontent.com/cailca/linux-mm/master/x86.config 

[  104.532621] UBSAN: array-index-out-of-bounds in arch/x86/mm/dump_pagetables.c:284:27
[  104.542620] index -1 is out of range for type 'pgprotval_t [5]'
[  104.552624] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-rc6-next-20200522+ #5
[  104.560865] Hardware name: HPE ProLiant DL385 Gen10/ProLiant DL385 Gen10, BIOS A40 07/10/2019
[  104.562604] Call Trace:
[  104.562604]  dump_stack+0xa7/0xea
[  104.562604]  ubsan_epilogue+0x9/0x45
[  104.562604]  __ubsan_handle_out_of_bounds.cold.12+0x2b/0x36
[  104.562604]  ? __kasan_check_write+0x14/0x20
[  104.562604]  note_page+0x91f/0xa00
[  104.562604]  ? down_read_non_owner+0x330/0x330
[  104.562604]  ? match_held_lock+0x20/0x250
[  104.562604]  ptdump_walk_pgd+0xa1/0xb0
[  104.562604]  ptdump_walk_pgd_level_core+0x19f/0x200
[  104.562604]  ? up+0x46/0x60
[  104.562604]  ? hugetlb_get_unmapped_area+0x590/0x590
[  104.562604]  ? lock_downgrade+0x3e0/0x3e0
[  104.562604]  ? _raw_spin_unlock_irqrestore+0x44/0x50
[  104.562604]  ? ptdump_walk_pgd_level_debugfs+0x80/0x80
[  104.562604]  ? ptdump_walk_pgd_level_core+0x200/0x200
[  104.562604]  ? virt_efi_set_variable_nonblocking+0xf1/0x110
[  104.562604]  ptdump_walk_pgd_level+0x32/0x40
[  104.562604]  efi_dump_pagetable+0x17/0x19
[  104.562604]  efi_enter_virtual_mode+0x3e5/0x3f5
[  104.562604]  start_kernel+0x848/0x8f6
[  104.562604]  ? __early_make_pgtable+0x2cb/0x314
[  104.562604]  ? thread_stack_cache_init+0xb/0xb
[  104.562604]  ? early_make_pgtable+0x21/0x23
[  104.562604]  ? early_idt_handler_common+0x35/0x4c
[  104.562604]  x86_64_start_reservations+0x24/0x26
[  104.562604]  x86_64_start_kernel+0xf4/0xfb
[  104.562604]  secondary_startup_64+0xb6/0xc0

>  
> -	if (level > 0) {
> -		new_eff = effective_prot(st->prot_levels[level - 1],
> -					 new_prot);
> -	} else {
> -		new_eff = new_prot;
> -	}
> -
> -	if (level >= 0)
> -		st->prot_levels[level] = new_eff;
> +	if (!val)
> +		new_eff = 0;
>  
>  	/*
>  	 * If we have a "break" in the series, we need to flush the state that