linux-kernel - Re: [PATCH v2 5/5] arm64: add KASan support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150714150445.GH13555@e104818-lin.cambridge.arm.com>
Date:	Tue, 14 Jul 2015 16:04:45 +0100
From:	Catalin Marinas <catalin.marinas@....com>
To:	Andrey Ryabinin <a.ryabinin@...sung.com>
Cc:	Arnd Bergmann <arnd@...db.de>,
	David Keitel <dkeitel@...eaurora.org>,
	Will Deacon <will.deacon@....com>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Alexander Potapenko <glider@...gle.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 5/5] arm64: add KASan support

On Fri, Jul 10, 2015 at 08:11:03PM +0300, Andrey Ryabinin wrote:
> >> +#if CONFIG_PGTABLE_LEVELS > 3
> >> +pud_t kasan_zero_pud[PTRS_PER_PUD] __page_aligned_bss;
> >> +#endif
> >> +#if CONFIG_PGTABLE_LEVELS > 2
> >> +pmd_t kasan_zero_pmd[PTRS_PER_PMD] __page_aligned_bss;
> >> +#endif
> >> +pte_t kasan_zero_pte[PTRS_PER_PTE] __page_aligned_bss;
> >> +
> >> +static void __init kasan_early_pmd_populate(unsigned long start,
> >> +					unsigned long end, pud_t *pud)
> >> +{
> >> +	unsigned long addr;
> >> +	unsigned long next;
> >> +	pmd_t *pmd;
> >> +
> >> +	pmd = pmd_offset(pud, start);
> >> +	for (addr = start; addr < end; addr = next, pmd++) {
> >> +		pmd_populate_kernel(&init_mm, pmd, kasan_zero_pte);
> >> +		next = pmd_addr_end(addr, end);
> >> +	}
> >> +}
> >> +
> >> +static void __init kasan_early_pud_populate(unsigned long start,
> >> +					unsigned long end, pgd_t *pgd)
> >> +{
> >> +	unsigned long addr;
> >> +	unsigned long next;
> >> +	pud_t *pud;
> >> +
> >> +	pud = pud_offset(pgd, start);
> >> +	for (addr = start; addr < end; addr = next, pud++) {
> >> +		pud_populate(&init_mm, pud, kasan_zero_pmd);
> >> +		next = pud_addr_end(addr, end);
> >> +		kasan_early_pmd_populate(addr, next, pud);
> >> +	}
> >> +}
> >> +
> >> +static void __init kasan_map_early_shadow(pgd_t *pgdp)
> >> +{
> >> +	int i;
> >> +	unsigned long start = KASAN_SHADOW_START;
> >> +	unsigned long end = KASAN_SHADOW_END;
> >> +	unsigned long addr;
> >> +	unsigned long next;
> >> +	pgd_t *pgd;
> >> +
> >> +	for (i = 0; i < PTRS_PER_PTE; i++)
> >> +		set_pte(&kasan_zero_pte[i], pfn_pte(
> >> +				virt_to_pfn(kasan_zero_page), PAGE_KERNEL));
> >> +
> >> +	pgd = pgd_offset_k(start);
> >> +	for (addr = start; addr < end; addr = next, pgd++) {
> >> +		pgd_populate(&init_mm, pgd, kasan_zero_pud);
> >> +		next = pgd_addr_end(addr, end);
> >> +		kasan_early_pud_populate(addr, next, pgd);
> >> +	}
> > 
> > I prefer to use "do ... while" constructs similar to __create_mapping()
> > (or zero_{pgd,pud,pmd}_populate as you are more familiar with them).
> > 
> > But what I don't get here is that you repopulate the pud page for every
> > pgd (and so on for pmd). You don't need this recursive call all the way
> > to kasan_early_pmd_populate() but just sequential:
> 
> This repopulation needed for 3,2 level page tables configurations.
> 
> E.g. for 3-level page tables we need to call pud_populate(&init_mm,
> pud, kasan_zero_pmd) for each pud in [KASAN_SHADOW_START,
> KASAN_SHADOW_END] range, this causes repopopulation for 4-level page
> tables, since we need to pud_populate() only [KASAN_SHADOW_START,
> KASAN_SHADOW_START + PGDIR_SIZE] range.

I'm referring to writing the same information multiple times over the
same entry. kasan_map_early_shadow() goes over each pgd entry and writes
the address of kasan_zero_pud. That's fine so far. However, in the same
loop you call kasan_early_pud_populate(). The latter retrieves the pud
page via pud_offset(pgd, start) which would always be kasan_zero_pud
because that's what you wrote via pgd_populate() in each pgd entry. So
for each pgd entry, you keep populating the same kasan_zero_pud page
with pointers to kasan_zero_pmd. And so on for the pmd.

> > 	kasan_early_pte_populate();
> > 	kasan_early_pmd_populate(..., pte);
> > 	kasan_early_pud_populate(..., pmd);
> > 	kasan_early_pgd_populate(..., pud);
> > 
> > (or in reverse order)
> 
> Unless, I'm missing something, this will either work only with 4-level
> page tables. We could do this without repopulation by using
> CONFIG_PGTABLE_LEVELS ifdefs.

Or you could move kasan_early_*_populate outside the loop. You already
do this for the pte at the beginning of the kasan_map_early_shadow()
function (and it probably makes more sense to create a separate
kasan_early_pte_populate).

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/