Re: [PATCH v2] kasan: fix runtime shadow page table synchronization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kasan: syncing static early shadow tables breaks kasan_remove_zero_shadow()
- [High] kasan: missing kasan_p*d_table() checks in kasan_populate_early_shadow()
--

commit 84e267a96cb1eab3b9fd041e188f62b5f7899dd0
Author: Muchun Song <muchun.song@linux.dev>

kasan: fix runtime shadow page table synchronization

This commit tracks page table modifications during KASAN zero-shadow
population and calls arch_sync_kernel_mappings() once after the walk. It also
avoids replacing already-populated page table entries during aligned shadow
population and retains page tables referenced by synchronized entries during
teardown.

> diff --git a/mm/kasan/init.c b/mm/kasan/init.c
> index 66a8838879876..f94f461d864ca 100644
> --- a/mm/kasan/init.c
> +++ b/mm/kasan/init.c

[ ... ]

> @@ -115,25 +116,26 @@ static int __ref zero_pmd_populate(pud_t *pud, unsigned long addr,
>  	do {
>  		next = pmd_addr_end(addr, end);
>  
> -		if (IS_ALIGNED(addr, PMD_SIZE) && end - addr >= PMD_SIZE) {
> +		if (IS_ALIGNED(addr, PMD_SIZE) && end - addr >= PMD_SIZE &&
> +		    pmd_none(*pmd)) {
>  			pmd_populate_kernel(&init_mm, pmd,
>  					lm_alias(kasan_early_shadow_pte));
> +			*mask |= PGTBL_PMD_MODIFIED;
>  			continue;
>  		}

[Severity: High]
What happens in zero_pmd_populate(), zero_pud_populate(), zero_p4d_populate(),
and kasan_populate_early_shadow() when the page table entry is not none
because it already points to the shared static early shadow table
(kasan_pte_table(*pmd), kasan_pmd_table(*pud), kasan_pud_table(*p4d), or
kasan_p4d_table(*pgd))?

For example, in arch/loongarch/mm/kasan_init.c, kasan_init() pre-populates
swapper_pg_dir across the entire shadow range with kasan_early_shadow_* before
calling kasan_populate_early_shadow():

arch/loongarch/mm/kasan_init.c:kasan_init() {
	/* Maps everything to a single page of zeroes */
	kasan_pgd_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, NUMA_NO_NODE, true);

	kasan_populate_early_shadow(mem_to_shadow((void *)VMALLOC_START),
					mem_to_shadow((void *)KFENCE_AREA_END));
}

Because *p4d, *pud, and *pmd point to kasan_early_shadow_pud,
kasan_early_shadow_pmd, and kasan_early_shadow_pte rather than
invalid_pud_table, invalid_pmd_table, and invalid_pte_table, p4d_none(*p4d),
pud_none(*pud), and pmd_none(*pmd) all evaluate to false.

Instead of skipping aligned PGDIR_SIZE, P4D_SIZE, PUD_SIZE, and PMD_SIZE
blocks, kasan_populate_early_shadow() descends into zero_pte_populate() for
every single PMD across the multi-terabyte VMALLOC_START..KFENCE_AREA_END
shadow range, executing billions of redundant set_pte_at() calls on the same
shared kasan_early_shadow_pte array (and on 16KB-page LoongArch configs where
mem_to_shadow(VMALLOC_START) is not PMD-aligned, prematurely write-protecting
kasan_early_shadow_pte entries while kasan_pg_dir is still active).

The same descent into the shared static kasan_early_shadow_* tables happens at
runtime if kasan_add_zero_shadow() -> kasan_populate_early_shadow() is called
again on a range where kasan_remove_zero_shadow() previously retained a
synchronized kasan_early_shadow_* entry.

Should these aligned fast paths also check whether the entry already points to
kasan_early_shadow_{p4d,pud,pmd,pte} (and skip descending without needing to
re-populate or set the modified mask)?

[ ... ]

> @@ -377,6 +420,9 @@ static void kasan_remove_pmd_table(pmd_t *pmd, unsigned long addr,
>  			continue;
>  
>  		if (kasan_pte_table(*pmd)) {
> +			if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PMD_MODIFIED)
> +				continue;
> +
>  			if (IS_ALIGNED(addr, PMD_SIZE) &&
>  			    IS_ALIGNED(next, PMD_SIZE)) {
>  				pmd_clear(pmd);
> @@ -403,6 +449,9 @@ static void kasan_remove_pud_table(pud_t *pud, unsigned long addr,
>  			continue;
>  
>  		if (kasan_pmd_table(*pud)) {
> +			if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PUD_MODIFIED)
> +				continue;
> +
>  			if (IS_ALIGNED(addr, PUD_SIZE) &&
>  			    IS_ALIGNED(next, PUD_SIZE)) {
>  				pud_clear(pud);
> @@ -430,6 +479,9 @@ static void kasan_remove_p4d_table(p4d_t *p4d, unsigned long addr,
>  			continue;
>  
>  		if (kasan_pud_table(*p4d)) {
> +			if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
> +				continue;
> +
>  			if (IS_ALIGNED(addr, P4D_SIZE) &&
>  			    IS_ALIGNED(next, P4D_SIZE)) {
>  				p4d_clear(p4d);
> @@ -464,6 +516,9 @@ void kasan_remove_zero_shadow(void *start, unsigned long size)
>  			continue;
>  
>  		if (kasan_p4d_table(*pgd)) {
> +			if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)
> +				continue;
> +
>  			if (IS_ALIGNED(addr, PGDIR_SIZE) &&
>  			    IS_ALIGNED(next, PGDIR_SIZE)) {
>  				pgd_clear(pgd);

[Severity: High]
Can skipping synchronized kasan_early_shadow_{p4d,pud,pmd,pte} entries here
prevent kasan_remove_zero_shadow() from unmapping the zero shadow at all?

When kasan_add_zero_shadow() -> kasan_populate_early_shadow() runs at runtime
(with slab_is_available() == true) for a range covering an aligned PGDIR_SIZE,
P4D_SIZE, PUD_SIZE, or PMD_SIZE boundary at a level included in
ARCH_PAGE_TABLE_SYNC_MASK (for example, PGTBL_P4D_MODIFIED on 4-level x86_64,
PGTBL_PGD_MODIFIED on 5-level x86_64, or PGTBL_PMD_MODIFIED on 32-bit x86/ARM),
it installs the shared static kasan_early_shadow_{p4d,pud,pmd,pte} table
directly into the synchronized entry.

When kasan_remove_zero_shadow() is later called to remove that shadow mapping,
hitting this continue leaves the entry pointing to kasan_early_shadow_*.
Unlike a dynamically allocated page table (where kasan_remove_*_table()
descends and clears all child entries before kasan_free_*() retains only the
empty top-level table), kasan_early_shadow_* is a shared static table whose
entries cannot be cleared and remain permanently mapped to the read-only
kasan_early_shadow_page.

If normal System RAM is subsequently hot-added and onlined in that address
range, kasan_mem_notifier() in mm/kasan/shadow.c checks shadow_mapped():

mm/kasan/shadow.c:kasan_mem_notifier() {
	...
	case MEM_GOING_ONLINE: {
		...
		if (shadow_mapped(shadow_start))
			return NOTIFY_OK;
}

Because the read-only kasan_early_shadow_page PTE is still present,
shadow_mapped() returns true and kasan_mem_notifier() skips allocating
writable shadow memory, causing subsequent KASAN shadow writes to fault on the
read-only zero shadow page. In addition, if a smaller or unaligned range
within that region is later passed to kasan_add_zero_shadow(),
kasan_populate_early_shadow() sees !p*d_none(*p*d) and descends into and
modifies the shared global kasan_early_shadow_* tables.

Should kasan_populate_early_shadow() avoid installing the static
kasan_early_shadow_{p4d,pud,pmd,pte} tables at page table levels that are in
ARCH_PAGE_TABLE_SYNC_MASK when slab_is_available() is true, and instead
allocate a normal page table at that synchronized level so its entries can be
cleared during kasan_remove_zero_shadow()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018043501.3257632-1-songmuchun@bytedance.com?part=1