linux-kernel - Re: [PATCH for-next/fixes] arm64/mm: Fix false-positive !virt_addr

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: <CAMj1kXFvJGHr_iv6bFQfb89XqPFrNWH7-rV7SFy4QBSWXYC4RA@mail.gmail.com>
Date: Sun, 24 Nov 2024 17:38:55 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Lukas Wunner <lukas@...ner.de>
Cc: Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, 
	Herbert Xu <herbert@...dor.apana.org.au>, Zorro Lang <zlang@...hat.com>, 
	Vegard Nossum <vegard.nossum@...cle.com>, Joey Gouly <joey.gouly@....com>, 
	linux-arm-kernel@...ts.infradead.org, linux-crypto@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH for-next/fixes] arm64/mm: Fix false-positive
 !virt_addr_valid() for kernel image

On Sun, 24 Nov 2024 at 17:16, Lukas Wunner <lukas@...ner.de> wrote:
>
> Zorro reports a false-positive BUG_ON() when running crypto selftests on
> boot:  Since commit 1e562deacecc ("crypto: rsassa-pkcs1 - Migrate to
> sig_alg backend"), test_sig_one() invokes an RSA verify operation with a
> test vector in the kernel's .rodata section.  The test vector is passed
> to sg_set_buf(), which performs a virt_addr_valid() check.
>
> On arm64, virt_addr_valid() returns false for kernel image addresses
> such as this one, even though they're valid virtual addresses.
> x86 returns true for kernel image addresses, so the BUG_ON() does not
> occur there.  In fact, x86 has been doing so for 16 years, i.e. since
> commit af5c2bd16ac2 ("x86: fix virt_addr_valid() with
> CONFIG_DEBUG_VIRTUAL=y, v2").
>
> Do the same on arm64 to avoid the false-positive BUG_ON() and to achieve
> consistent virt_addr_valid() behavior across arches.
>
> Silence a WARN splat in __virt_to_phys() which occurs once the BUG_ON()
> is avoided.
>
> The is_kernel_address() helper introduced herein cannot be put directly
> in the virt_addr_valid() macro:  It has to be part of the kernel proper
> so that it has visibility of the _text and _end symbols (referenced
> through KERNEL_START and KERNEL_END).  These symbols are not exported,
> so modules expanding the virt_addr_valid() macro could not access them.
> For almost all invocations of virt_addr_valid(), __is_lm_address()
> returns true, so jumping to the is_kernel_address() helper hardly ever
> occurs and its performance impact is thus negligible.
>
> Likewise, calling is_kernel_address() from the functions in physaddr.c
> ought to be fine as they depend on CONFIG_DEBUG_VIRTUAL=y, which is
> explicitly described as "costly" in the Kconfig help text.  (And this
> doesn't add much cost really.)
>
> Abridged stack trace:
>
>   kernel BUG at include/linux/scatterlist.h:187!
>   sg_init_one()
>   rsassa_pkcs1_verify()
>   test_sig_one()
>   alg_test_sig()
>   alg_test()
>   cryptomgr_test()
>
> Fixes: 1e562deacecc ("crypto: rsassa-pkcs1 - Migrate to sig_alg backend")
> Reported-by: Zorro Lang <zlang@...hat.com>
> Closes: https://lore.kernel.org/r/20241122045106.tzhvm2wrqvttub6k@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com/
> Signed-off-by: Lukas Wunner <lukas@...ner.de>
> ---
> Just from looking at the code it seems arm's virt_addr_valid() returns
> true for kernel image addresses, so apparently arm64 is the odd man out.
>

That is because ARM maps the kernel in the linear map, whereas arm64
maps the kernel in the vmalloc space.

vmalloc addresses cannot be used for DMA, which is why
virt_addr_valid() rejects them. On arm64, the same applies to the
kernel image, as well as the vmap'ed stack.

> Note that this fix would have obviated the need for commit c02e7c5c6da8
> ("arm64/mm: use lm_alias() with addresses passed to memblock_free()").
>

Your 'fix' will break other stuff: it is used, e.g., to decide whether
__pa() may be used on the input VA, which applies a fixed translation
on the input, rather than walk the page tables to obtain the physical
address.


>  arch/arm64/include/asm/memory.h | 6 +++++-
>  arch/arm64/mm/init.c            | 7 +++++++
>  arch/arm64/mm/physaddr.c        | 6 +++---
>  3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index b9b9929..bb83315 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -416,9 +416,13 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
>  })
>  #endif /* CONFIG_DEBUG_VIRTUAL */
>
> +bool is_kernel_address(unsigned long x);
> +
>  #define virt_addr_valid(addr)  ({                                      \
>         __typeof__(addr) __addr = __tag_reset(addr);                    \
> -       __is_lm_address(__addr) && pfn_is_map_memory(virt_to_pfn(__addr));      \
> +       (__is_lm_address(__addr) ||                                     \
> +        is_kernel_address((unsigned long)__addr)) &&                   \
> +       pfn_is_map_memory(virt_to_pfn(__addr));                         \
>  })
>
>  void dump_mem_limit(void);
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index d21f67d..2e8a00f 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -156,6 +156,13 @@ static void __init zone_sizes_init(void)
>         free_area_init(max_zone_pfns);
>  }
>
> +bool is_kernel_address(unsigned long x)
> +{
> +       return x >= (unsigned long) KERNEL_START &&
> +              x <= (unsigned long) KERNEL_END;
> +}
> +EXPORT_SYMBOL(is_kernel_address);
> +
>  int pfn_is_map_memory(unsigned long pfn)
>  {
>         phys_addr_t addr = PFN_PHYS(pfn);
> diff --git a/arch/arm64/mm/physaddr.c b/arch/arm64/mm/physaddr.c
> index cde44c1..2d6755b 100644
> --- a/arch/arm64/mm/physaddr.c
> +++ b/arch/arm64/mm/physaddr.c
> @@ -9,7 +9,8 @@
>
>  phys_addr_t __virt_to_phys(unsigned long x)
>  {
> -       WARN(!__is_lm_address(__tag_reset(x)),
> +       WARN(!__is_lm_address(__tag_reset(x)) &&
> +            !is_kernel_address(__tag_reset(x)),
>              "virt_to_phys used for non-linear address: %pK (%pS)\n",
>               (void *)x,
>               (void *)x);
> @@ -24,8 +25,7 @@ phys_addr_t __phys_addr_symbol(unsigned long x)
>          * This is bounds checking against the kernel image only.
>          * __pa_symbol should only be used on kernel symbol addresses.
>          */
> -       VIRTUAL_BUG_ON(x < (unsigned long) KERNEL_START ||
> -                      x > (unsigned long) KERNEL_END);
> +       VIRTUAL_BUG_ON(!is_kernel_address(x));
>         return __pa_symbol_nodebug(x);
>  }
>  EXPORT_SYMBOL(__phys_addr_symbol);
> --
> 2.43.0
>
>