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] [day] [month] [year] [list]

Message-ID: <CAMj1kXER7AbNyUDjtij6Ni0jVRMg11xvyhkCMKAxaKbx=dsgcQ@mail.gmail.com>
Date: Sun, 24 Nov 2024 18:13:26 +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:38, Ard Biesheuvel <ardb@...nel.org> wrote:
>
> 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.
>

Apologies, I replied a little too quickly.

__pa() on arm64 permits linear addresses and kernel image addresses,
and so the kernel image is treated differently from the vmap'ed stack
and other vmalloc addresses.

However, that doesn't mean doing DMA from the kernel image is a great
idea. Allocations in the linear map are rounded up to cacheline size
to ensure that they are safe for non-coherent DMA, but this does not
apply to the kernel image. .rodata should still be safe in this
regard, but the general idea of allowing kernel image addresses in
places where DMA'able virtual addresses are expected is something we
should consider with care.