Re: [PATCH] x86/mm: Fix possible index overflow when creating page table mapping

From: "Kirill A. Shutemov" <kirill@shutemov.name>
To: Yuntao Wang <ytcoode@gmail.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>,
	Andy Lutomirski <luto@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>, Baoquan He <bhe@redhat.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH] x86/mm: Fix possible index overflow when creating page table mapping
Date: Sat, 18 Jun 2022 03:22:20 +0300	[thread overview]
Message-ID: <20220618002220.panwk4zwnigayboy@box.shutemov.name> (raw)
In-Reply-To: <20220616135510.1784995-1-ytcoode@gmail.com>

On Thu, Jun 16, 2022 at 09:55:10PM +0800, Yuntao Wang wrote:
> There are two issues in phys_p4d_init():
> 
> - The __kernel_physical_mapping_init() does not do boundary-checking for
>   paddr_end and passes it directly to phys_p4d_init(), phys_p4d_init() does
>   not do bounds checking either, so if the physical memory to be mapped is
>   large enough, 'p4d_page + p4d_index(vaddr)' will wrap around to the
>   beginning entry of the P4D table and its data will be overwritten.
> 
> - The for loop body will be executed only when 'vaddr < vaddr_end'
>   evaluates to true, but if that condition is true, 'paddr >= paddr_end'
>   will evaluate to false, thus the 'if (paddr >= paddr_end) {}' block will
>   never be executed and become dead code.
> 
> To fix these issues, use 'i < PTRS_PER_P4D' instead of 'vaddr < vaddr_end'
> as the for loop condition, this also make it more consistent with the logic
> of the phys_{pud,pmt,pte}_init() functions.

Hm. I don't see why you changed phys_p4d_init(), but not
__kernel_physical_mapping_init(). It does exactly the same thing, just
pgd_index() is hidden a bit deeper than p4d_index().

-- 
 Kirill A. Shutemov