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

[Andrey Ryabinin]

Muchun Song <songmuchun@xxxxxxxxxxxxx> writes:

> kasan_populate_early_shadow() is also used after slab becomes available
> to install shadow mappings for hot-added memory. When this path
> allocates a new P4D or PUD table, p4d_alloc() and pud_alloc() publish it
> only in init_mm.
>
> On x86-64, runtime kernel mappings that add a top-level entry must call
> arch_sync_kernel_mappings() so existing process page tables can see it.
> Without this synchronization, the KASAN shadow mapping can remain
> unreachable and fault when the hot-added memory is accessed.
>

Did you actually hit this crash or it's just LLM's/code inspection finding?

> Add p4d_alloc_kernel() and pud_alloc_kernel() to perform the required
> synchronization, and use them for runtime KASAN shadow population.
>
> Once copied, a top-level entry remains in other process page tables.
> Clearing only init_mm and freeing its target table during teardown would
> leave stale pointers behind. Retain these shared levels instead. On x86,
> this keeps PUD tables with four-level paging and P4D tables with
> five-level paging, while lower-level tables can still be freed.
>
> Fixes: 0207df4fa1a8 ("kernel/memremap, kasan: make ZONE_DEVICE with work with KASAN")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> ---
>  include/linux/mm.h | 24 ++++++++++++++++++++++++
>  mm/kasan/init.c    | 10 ++++++++--
>  2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 5cca90fc1df6..5453d3e42780 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3855,6 +3855,18 @@ static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd,
>  		NULL : p4d_offset(pgd, address);
>  }
>
> +static inline p4d_t *p4d_alloc_kernel(pgd_t *pgd, unsigned long address)
> +{
> +	if (unlikely(pgd_none(*pgd))) {
> +		if (__p4d_alloc(&init_mm, pgd, address))
> +			return NULL;
> +		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_PGD_MODIFIED)
> +			arch_sync_kernel_mappings(address, address);
> +	}
> +
> +	return p4d_offset(pgd, address);
> +}
> +
>  static inline pud_t *pud_alloc(struct mm_struct *mm, p4d_t *p4d,
>  		unsigned long address)
>  {
> @@ -3862,6 +3874,18 @@ static inline pud_t *pud_alloc(struct mm_struct *mm, p4d_t *p4d,
>  		NULL : pud_offset(p4d, address);
>  }
>
> +static inline pud_t *pud_alloc_kernel(p4d_t *p4d, unsigned long address)
> +{
> +	if (unlikely(p4d_none(*p4d))) {
> +		if (__pud_alloc(&init_mm, p4d, address))
> +			return NULL;
> +		if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
> +			arch_sync_kernel_mappings(address, address);
> +	}
> +
> +	return pud_offset(p4d, address);
> +}
> +
>  static inline pmd_t *pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address)
>  {
>  	return (unlikely(pud_none(*pud)) && __pmd_alloc(mm, pud, address))?
> diff --git a/mm/kasan/init.c b/mm/kasan/init.c
> index 66a883887987..93a74a1fcdec 100644
> --- a/mm/kasan/init.c
> +++ b/mm/kasan/init.c
> @@ -206,7 +206,7 @@ static int __ref zero_p4d_populate(pgd_t *pgd, unsigned long addr,
>  			pud_t *p;
>
>  			if (slab_is_available()) {
> -				p = pud_alloc(&init_mm, p4d, addr);
> +				p = pud_alloc_kernel(p4d, addr);

I think it would be better to reuse pud_alloc_track() here.
Pass a pgtbl_mod_mask through zero_p4d/pud/pmd_populate(), and call
arch_sync_kernel_mappings()
only once at the end of kasan_populate_early_shadow()

>  				if (!p)
>  					return -ENOMEM;
>  			} else {
> @@ -268,7 +268,7 @@ int __ref kasan_populate_early_shadow(const void *shadow_start,
>  		if (pgd_none(*pgd)) {
>
>  			if (slab_is_available()) {
> -				if (!p4d_alloc(&init_mm, pgd, addr))
> +				if (!p4d_alloc_kernel(pgd, addr))

Ditto

>  					return -ENOMEM;
>  			} else {
>  				pgd_populate_kernel(addr, pgd,
> @@ -322,6 +322,9 @@ static void kasan_free_pud(pud_t *pud_start, p4d_t *p4d)
>  			return;
>  	}
>
> +	if (ARCH_PAGE_TABLE_SYNC_MASK & PGTBL_P4D_MODIFIED)
> +		return;
> +

With 4-level paging the PUD table is now kept here after it has been
synced to every pgd. A later kasan_add_zero_shadow() call can overwrite
it in init_mm only. This might cause a problem, because the aligned
shortcuts in kasan_populate_early_shadow() and zero_p4d_populate()
don't check whether the entry is already populated:

mm/kasan/init.c:kasan_populate_early_shadow() {
    ...
                if (IS_ALIGNED(addr, PGDIR_SIZE) && end - addr >= PGDIR_SIZE) {
            ...
                        p4d_populate_kernel(addr, p4d,
                                        lm_alias(kasan_early_shadow_pud));
    ...
}

So sequence of kasan_add_zero_shadow(), kasan_remove_zero_shadow(),
kasan_add_zero_shadow(), arch_sync_kernel_mappings()
might trigger the BUG_ON() in sync_global_pgds_l4():

arch/x86/mm/init_64.c:sync_global_pgds_l4() {
    ...
                        if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))
                                BUG_ON(p4d_pgtable(*p4d)
                                       != p4d_pgtable(*p4d_ref));
    ...
}

Perhaps the aligned populate paths should skip an entry that is
already populated.

>  	pud_free(&init_mm, pud_start);
>  	p4d_clear(p4d);
>  }