Re: [RFC PATCH] arm64: don't vmap() invalid page

[Mark Rutland]

On Tue, Jan 18, 2022 at 10:53:54AM -0800, Yury Norov wrote:
> vmap() takes struct page *pages as one of arguments, and user may provide
> an invalid pointer, which would lead to DABT at address translation later.
> Currently, kernel checks the pages against NULL. In my case, however, the
> address was not NULL, and was big enough so that the hardware generated
> Address Size Abort.

Can you give an example of when this might happen? It sounds like you're
actually hitting this, so a backtrace would be nice.

I'm a bit confused as to when why we'd try to vmap() pages that we
didn't have a legitimate struct page for -- where did these addresses
come from?

It sounds like this is going wrong at a higher level, and we're passing
entirely bogus struct page pointers around. This seems like the sort of
thing DEBUG_VIRTUAL or similar should check when we initially generate
the struct page pointer.

Thanks,
Mark.

> Interestingly, this abort happens even if copy_from_kernel_nofault() is used,
> which is quite inconvenient for debugging purposes. 
> 
> This patch adds an arch_vmap_page_valid() helper into vmap() path, so that
> architectures may add arch-specific checks of the pointer passed into vmap.
> 
> For arm64, if the page passed to vmap() corresponds to a physical address
> greater than maximum possible value as described in TCR_EL1.IPS register, the
> following table walk would generate Address Size Abort. Instead of creating
> the invalid mapping, kernel will return ERANGE in such situation.
> 
> Signed-off-by: Yury Norov <yury.norov@xxxxxxxxx>
> ---
>  arch/arm64/include/asm/vmalloc.h | 41 ++++++++++++++++++++++++++++++++
>  include/linux/vmalloc.h          |  7 ++++++
>  mm/vmalloc.c                     |  8 +++++--
>  3 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/vmalloc.h b/arch/arm64/include/asm/vmalloc.h
> index b9185503feae..e9d43ee019ad 100644
> --- a/arch/arm64/include/asm/vmalloc.h
> +++ b/arch/arm64/include/asm/vmalloc.h
> @@ -4,6 +4,47 @@
>  #include <asm/page.h>
>  #include <asm/pgtable.h>
>  
> +static inline u64 pa_size(u64 ips)
> +{
> +	switch (ips) {
> +	case 0b000:
> +		return 1UL << 32;
> +	case 0b001:
> +		return 1UL << 36;
> +	case 0b010:
> +		return 1UL << 40;
> +	case 0b011:
> +		return 1UL << 42;
> +	case 0b100:
> +		return 1UL << 44;
> +	case 0b101:
> +		return 1UL << 48;
> +	case 0b110:
> +		return 1UL << 52;
> +	/* All other values */
> +	default:
> +		return 1UL << 52;
> +	}
> +}
> +
> +#define arch_vmap_page_valid arch_vmap_page_valid
> +static inline int arch_vmap_page_valid(struct page *page)
> +{
> +	u64 tcr, ips, paddr_size;
> +
> +	if (!page)
> +		return -ENOMEM;
> +
> +	tcr = read_sysreg_s(SYS_TCR_EL1);
> +	ips = (tcr & TCR_IPS_MASK) >> TCR_IPS_SHIFT;
> +
> +	paddr_size = pa_size(ips);
> +	if (page_to_phys(page) >= paddr_size)
> +		return -ERANGE;
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
>  
>  #define arch_vmap_pud_supported arch_vmap_pud_supported
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index 6e022cc712e6..08b567d8bafc 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -119,6 +119,13 @@ static inline int arch_vmap_pte_supported_shift(unsigned long size)
>  }
>  #endif
>  
> +#ifndef arch_vmap_page_valid
> +static inline int arch_vmap_page_valid(struct page *page)
> +{
> +	return page ? 0 : -ENOMEM;
> +}
> +#endif
> +
>  /*
>   *	Highlevel APIs for driver use
>   */
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index d2a00ad4e1dd..ee0384405cdd 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -472,11 +472,15 @@ static int vmap_pages_pte_range(pmd_t *pmd, unsigned long addr,
>  		return -ENOMEM;
>  	do {
>  		struct page *page = pages[*nr];
> +		int ret;
>  
>  		if (WARN_ON(!pte_none(*pte)))
>  			return -EBUSY;
> -		if (WARN_ON(!page))
> -			return -ENOMEM;
> +
> +		ret = arch_vmap_page_valid(page);
> +		if (WARN_ON(ret))
> +			return ret;
> +
>  		set_pte_at(&init_mm, addr, pte, mk_pte(page, prot));
>  		(*nr)++;
>  	} while (pte++, addr += PAGE_SIZE, addr != end);
> -- 
> 2.30.2
>