[PATCH] rwonce: fix crash by removing READ_ONCE() for unaligned read

[PATCH] rwonce: fix crash by removing READ_ONCE() for unaligned read

[Jann Horn]

When arm64 is built with LTO, it upgrades READ_ONCE() to ldar / ldapr
(load-acquire) to avoid issues that can be caused by the compiler
optimizing away implicit address dependencies.

Unlike plain loads, these load-acquire instructions actually require an
aligned address.

For now, fix it by removing the READ_ONCE() that the buggy commit
introduced.

Fixes: ece69af2ede1 ("rwonce: handle KCSAN like KASAN in read_word_at_a_time()")
Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://lore.kernel.org/r/20250326203926.GA10484@ax162
Signed-off-by: Jann Horn <jannh@google.com>
---
 include/asm-generic/rwonce.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
index e9f2b84d2338..52b969c7cef9 100644
--- a/include/asm-generic/rwonce.h
+++ b/include/asm-generic/rwonce.h
@@ -86,7 +86,12 @@ unsigned long read_word_at_a_time(const void *addr)
 	kasan_check_read(addr, 1);
 	kcsan_check_read(addr, 1);
 
-	return READ_ONCE(*(unsigned long *)addr);
+	/*
+	 * This load can race with concurrent stores to out-of-bounds memory,
+	 * but READ_ONCE() can't be used because it requires higher alignment
+	 * than plain loads in arm64 builds with LTO.
+	 */
+	return *(unsigned long *)addr;
 }
 
 #endif /* __ASSEMBLY__ */

---
base-commit: ece69af2ede103e190ffdfccd9f9ec850606ab5e
change-id: 20250326-rwaat-fix-63d7557b3d88

-- 
Jann Horn <jannh@google.com>

Re: [PATCH] rwonce: fix crash by removing READ_ONCE() for unaligned read

[Arnd Bergmann]

On Wed, Mar 26, 2025, at 22:04, Jann Horn wrote:
> When arm64 is built with LTO, it upgrades READ_ONCE() to ldar / ldapr
> (load-acquire) to avoid issues that can be caused by the compiler
> optimizing away implicit address dependencies.
>
> Unlike plain loads, these load-acquire instructions actually require an
> aligned address.
>
> For now, fix it by removing the READ_ONCE() that the buggy commit
> introduced.
>
> Fixes: ece69af2ede1 ("rwonce: handle KCSAN like KASAN in read_word_at_a_time()")
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Closes: https://lore.kernel.org/r/20250326203926.GA10484@ax162
> Signed-off-by: Jann Horn <jannh@google.com>

Thanks for the quick fix!

I've applied this on top of the asm-generic branch, but I just sent
the pull request with the regression to Linus an hour ago.

I'll try to get a new pull request out tomorrow.

      Arnd

> ---
>  include/asm-generic/rwonce.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
> index e9f2b84d2338..52b969c7cef9 100644
> --- a/include/asm-generic/rwonce.h
> +++ b/include/asm-generic/rwonce.h
> @@ -86,7 +86,12 @@ unsigned long read_word_at_a_time(const void *addr)
>  	kasan_check_read(addr, 1);
>  	kcsan_check_read(addr, 1);
> 
> -	return READ_ONCE(*(unsigned long *)addr);
> +	/*
> +	 * This load can race with concurrent stores to out-of-bounds memory,
> +	 * but READ_ONCE() can't be used because it requires higher alignment
> +	 * than plain loads in arm64 builds with LTO.
> +	 */
> +	return *(unsigned long *)addr;
>  }
> 
>  #endif /* __ASSEMBLY__ */
>
> ---
> base-commit: ece69af2ede103e190ffdfccd9f9ec850606ab5e
> change-id: 20250326-rwaat-fix-63d7557b3d88
>
> -- 
> Jann Horn <jannh@google.com>

Re: [PATCH] rwonce: fix crash by removing READ_ONCE() for unaligned read

[Linus Torvalds]

On Wed, 26 Mar 2025 at 14:24, Arnd Bergmann <arnd@arndb.de> wrote:
>
> I've applied this on top of the asm-generic branch, but I just sent
> the pull request with the regression to Linus an hour ago.
>
> I'll try to get a new pull request out tomorrow.

I will ignore that pull request, and wait for your updated one.

That said, this whole thing worries me. The fact that the compiler
magically makes READ_ONCE() require alignment that it normally doesn't
require seems like a bug waiting to happen somewhere else.

Because I do think that we might want READ_ONCE() on unaligned data in
general. Should said places generally use "get_unaligned()"? Sure. And
are unaligned accesses potentially non-atomic anyway because of
hardware? Also sure.

But one reason for READ_ONCE() isn't for some kind of hardware
atomicity, it is to avoid any ToCToU issues due to compilers doing bad
things.

And then this seems to be a serious issue with the whole "READ_ONCE()
now requires alignment that it didn't require before".

Put another way: I wonder what other cases may lurk around this all...

           Linus

Re: [PATCH] rwonce: fix crash by removing READ_ONCE() for unaligned read

[Jann Horn]

On Wed, Mar 26, 2025 at 11:41 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, 26 Mar 2025 at 14:24, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > I've applied this on top of the asm-generic branch, but I just sent
> > the pull request with the regression to Linus an hour ago.
> >
> > I'll try to get a new pull request out tomorrow.
>
> I will ignore that pull request, and wait for your updated one.
>
> That said, this whole thing worries me. The fact that the compiler
> magically makes READ_ONCE() require alignment that it normally doesn't
> require seems like a bug waiting to happen somewhere else.

To be clear, this is not the compiler's doing. The arch-specific arm64
code defines a custom version of the __READ_ONCE() macro used by
READ_ONCE() in LTO builds:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/rwonce.h

/*
 * When building with LTO, there is an increased risk of the compiler
 * converting an address dependency headed by a READ_ONCE() invocation
 * into a control dependency and consequently allowing for harmful
 * reordering by the CPU.
 *
 * Ensure that such transformations are harmless by overriding the generic
 * READ_ONCE() definition with one that provides RCpc acquire semantics
 * when building with LTO.
 */

Re: [PATCH] rwonce: fix crash by removing READ_ONCE() for unaligned read

[Nathan Chancellor]

On Wed, Mar 26, 2025 at 03:41:34PM -0700, Linus Torvalds wrote:
> That said, this whole thing worries me. The fact that the compiler
> magically makes READ_ONCE() require alignment that it normally doesn't
> require seems like a bug waiting to happen somewhere else.

For the record, I do not think it is the compiler doing this, it is the
arm64 code after commit e35123d83ee3 ("arm64: lto: Strengthen
READ_ONCE() to acquire when CONFIG_LTO=y") back in 5.11.

> Because I do think that we might want READ_ONCE() on unaligned data in
> general. Should said places generally use "get_unaligned()"? Sure. And
> are unaligned accesses potentially non-atomic anyway because of
> hardware? Also sure.
> 
> But one reason for READ_ONCE() isn't for some kind of hardware
> atomicity, it is to avoid any ToCToU issues due to compilers doing bad
> things.
> 
> And then this seems to be a serious issue with the whole "READ_ONCE()
> now requires alignment that it didn't require before".
> 
> Put another way: I wonder what other cases may lurk around this all...

That change has caused only one issue that I know of, which was fixed by
commit d3f450533bbc ("efi: tpm: Avoid READ_ONCE() for accessing the
event log"). I have not seen any since then until this point and I do
daily boots of -next with LTO enabled on both of my arm64 test machines.

Cheers,
Nathan

Re: [PATCH] rwonce: fix crash by removing READ_ONCE() for unaligned read

[Linus Torvalds]

On Wed, 26 Mar 2025 at 15:54, Nathan Chancellor <nathan@kernel.org> wrote:
>
> > Put another way: I wonder what other cases may lurk around this all...
>
> That change has caused only one issue that I know of, which was fixed by
> commit d3f450533bbc ("efi: tpm: Avoid READ_ONCE() for accessing the
> event log"). I have not seen any since then until this point and I do
> daily boots of -next with LTO enabled on both of my arm64 test machines.

Ahh, ok. That makes me happier.

I guess unaligned READ_ONCE() code really shouldn't exist in generic
code anyway, since some architectures will fail any unaligned access.

But those architectures tend to not get a lot of testing (they are a
dying breed - good riddance), so "shouldn't exist" doesn't necessarily
equate to really not existing.

          Linus

Re: [PATCH] rwonce: fix crash by removing READ_ONCE() for unaligned read

[Arnd Bergmann]

On Thu, Mar 27, 2025, at 01:49, Linus Torvalds wrote:
> On Wed, 26 Mar 2025 at 15:54, Nathan Chancellor <nathan@kernel.org> wrote:
>>
>> > Put another way: I wonder what other cases may lurk around this all...
>>
>> That change has caused only one issue that I know of, which was fixed by
>> commit d3f450533bbc ("efi: tpm: Avoid READ_ONCE() for accessing the
>> event log"). I have not seen any since then until this point and I do
>> daily boots of -next with LTO enabled on both of my arm64 test machines.
>
> Ahh, ok. That makes me happier.

I've sent a new v2 pull request now.

> I guess unaligned READ_ONCE() code really shouldn't exist in generic
> code anyway, since some architectures will fail any unaligned access.

Even if the unaligned READ_ONCE()/WRITE_ONCE() doesn't fail, it may
be surprising to callers when it is not atomic.

> But those architectures tend to not get a lot of testing (they are a
> dying breed - good riddance), so "shouldn't exist" doesn't necessarily
> equate to really not existing.

Unfortunately, they don't seem to quite die out just yet, as both
riscv and loongarch have gained support for CPUs without unaligned
access even though they started out requiring it:

https://lore.kernel.org/lkml/20231004151405.521596-1-cleger@rivosinc.com/
https://lore.kernel.org/lkml/20230202084238.2408516-1-chenhuacai@loongson.cn/

ARMv7 also has the annoying behavior of supporting unaligned word
access, but not unaligned multi-word load/store with ldrd/strd
on u64 variables.

     Arnd