rwonce: handle KCSAN like KASAN in read_word_at_a_time()

[PATCH] rwonce: handle KCSAN like KASAN in read_word_at_a_time()

Posted by Jann Horn 8 months, 3 weeks ago

read_word_at_a_time() is allowed to read out of bounds by straddling the
end of an allocation (and the caller is expected to then mask off
out-of-bounds data). This works as long as the caller guarantees that the
access won't hit a pagefault (either by ensuring that addr is aligned or by
explicitly checking where the next page boundary is).

Such out-of-bounds data could include things like KASAN redzones, adjacent
allocations that are concurrently written to, or simply an adjacent struct
field that is concurrently updated. KCSAN should ignore racy reads of OOB
data that is not actually used, just like KASAN, so (similar to the code
above) change read_word_at_a_time() to use __no_sanitize_or_inline instead
of __no_kasan_or_inline, and explicitly inform KCSAN that we're reading
the first byte.

We do have an instrument_read() helper that calls into both KASAN and
KCSAN, but I'm instead open-coding that here to avoid having to pull the
entire instrumented.h header into rwonce.h.

Also, since this read can be racy by design, we should technically do
READ_ONCE(), so add that.

Fixes: dfd402a4c4ba ("kcsan: Add Kernel Concurrency Sanitizer infrastructure")
Signed-off-by: Jann Horn <jannh@google.com>
---
This is a low-priority fix. I've never actually hit this issue with
upstream KCSAN.
(I only noticed it because I... err... hooked up KASAN to the KCSAN
hooks. Long story.)

I'm not sure if this should go through Arnd's tree (because it's in
rwonce.h) or Marco's (because it's a KCSAN thing).
Going through Marco's tree (after getting an Ack from Arnd) might
work a little better for me, I may or may not have more KCSAN patches
in the future.
---
 include/asm-generic/rwonce.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
index 8d0a6280e982..e9f2b84d2338 100644
--- a/include/asm-generic/rwonce.h
+++ b/include/asm-generic/rwonce.h
@@ -79,11 +79,14 @@ unsigned long __read_once_word_nocheck(const void *addr)
 	(typeof(x))__read_once_word_nocheck(&(x));			\
 })
 
-static __no_kasan_or_inline
+static __no_sanitize_or_inline
 unsigned long read_word_at_a_time(const void *addr)
 {
+	/* open-coded instrument_read(addr, 1) */
 	kasan_check_read(addr, 1);
-	return *(unsigned long *)addr;
+	kcsan_check_read(addr, 1);
+
+	return READ_ONCE(*(unsigned long *)addr);
 }
 
 #endif /* __ASSEMBLY__ */

---
base-commit: 2df0c02dab829dd89360d98a8a1abaa026ef5798
change-id: 20250325-kcsan-rwonce-c990e2c1fca5

-- 
Jann Horn <jannh@google.com>

Re: [PATCH] rwonce: handle KCSAN like KASAN in read_word_at_a_time()

Posted by Nathan Chancellor 8 months, 3 weeks ago

Hi Jann,

On Tue, Mar 25, 2025 at 05:01:34PM +0100, Jann Horn wrote:
> Also, since this read can be racy by design, we should technically do
> READ_ONCE(), so add that.
> 
> Fixes: dfd402a4c4ba ("kcsan: Add Kernel Concurrency Sanitizer infrastructure")
> Signed-off-by: Jann Horn <jannh@google.com>
...
> diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
> index 8d0a6280e982..e9f2b84d2338 100644
> --- a/include/asm-generic/rwonce.h
> +++ b/include/asm-generic/rwonce.h
> @@ -79,11 +79,14 @@ unsigned long __read_once_word_nocheck(const void *addr)
>  	(typeof(x))__read_once_word_nocheck(&(x));			\
>  })
>  
> -static __no_kasan_or_inline
> +static __no_sanitize_or_inline
>  unsigned long read_word_at_a_time(const void *addr)
>  {
> +	/* open-coded instrument_read(addr, 1) */
>  	kasan_check_read(addr, 1);
> -	return *(unsigned long *)addr;
> +	kcsan_check_read(addr, 1);
> +
> +	return READ_ONCE(*(unsigned long *)addr);

I bisected a boot hang that I see on arm64 with LTO enabled to this
change as commit ece69af2ede1 ("rwonce: handle KCSAN like KASAN in
read_word_at_a_time()") in -next. With LTO, READ_ONCE() gets upgraded to
ldar / ldapr, which requires an aligned address to access, but
read_word_at_a_time() can be called with an unaligned address. I
confirmed this should be the root cause by removing the READ_ONCE()
added above or removing the selects of DCACHE_WORD_ACCESS and
HAVE_EFFICIENT_UNALIGNED_ACCESS in arch/arm64/Kconfig, which avoids
the crash.

Cheers,
Nathan

Re: [PATCH] rwonce: handle KCSAN like KASAN in read_word_at_a_time()

Posted by Jann Horn 8 months, 3 weeks ago

On Wed, Mar 26, 2025 at 9:39 PM Nathan Chancellor <nathan@kernel.org> wrote:
> On Tue, Mar 25, 2025 at 05:01:34PM +0100, Jann Horn wrote:
> > Also, since this read can be racy by design, we should technically do
> > READ_ONCE(), so add that.
> >
> > Fixes: dfd402a4c4ba ("kcsan: Add Kernel Concurrency Sanitizer infrastructure")
> > Signed-off-by: Jann Horn <jannh@google.com>
> ...
> > diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
> > index 8d0a6280e982..e9f2b84d2338 100644
> > --- a/include/asm-generic/rwonce.h
> > +++ b/include/asm-generic/rwonce.h
> > @@ -79,11 +79,14 @@ unsigned long __read_once_word_nocheck(const void *addr)
> >       (typeof(x))__read_once_word_nocheck(&(x));                      \
> >  })
> >
> > -static __no_kasan_or_inline
> > +static __no_sanitize_or_inline
> >  unsigned long read_word_at_a_time(const void *addr)
> >  {
> > +     /* open-coded instrument_read(addr, 1) */
> >       kasan_check_read(addr, 1);
> > -     return *(unsigned long *)addr;
> > +     kcsan_check_read(addr, 1);
> > +
> > +     return READ_ONCE(*(unsigned long *)addr);
>
> I bisected a boot hang that I see on arm64 with LTO enabled to this
> change as commit ece69af2ede1 ("rwonce: handle KCSAN like KASAN in
> read_word_at_a_time()") in -next. With LTO, READ_ONCE() gets upgraded to
> ldar / ldapr, which requires an aligned address to access, but
> read_word_at_a_time() can be called with an unaligned address. I
> confirmed this should be the root cause by removing the READ_ONCE()
> added above or removing the selects of DCACHE_WORD_ACCESS and
> HAVE_EFFICIENT_UNALIGNED_ACCESS in arch/arm64/Kconfig, which avoids
> the crash.

Oh, bleeeh. Thanks for figuring that out. I guess that means we should
remove that READ_ONCE() again to un-break the build. I'll send a patch
in a bit...

Re: [PATCH] rwonce: handle KCSAN like KASAN in read_word_at_a_time()

Posted by Jann Horn 8 months, 3 weeks ago

On Wed, Mar 26, 2025 at 9:44 PM Jann Horn <jannh@google.com> wrote:
> On Wed, Mar 26, 2025 at 9:39 PM Nathan Chancellor <nathan@kernel.org> wrote:
> > On Tue, Mar 25, 2025 at 05:01:34PM +0100, Jann Horn wrote:
> > > Also, since this read can be racy by design, we should technically do
> > > READ_ONCE(), so add that.
> > >
> > > Fixes: dfd402a4c4ba ("kcsan: Add Kernel Concurrency Sanitizer infrastructure")
> > > Signed-off-by: Jann Horn <jannh@google.com>
> > ...
> > > diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
> > > index 8d0a6280e982..e9f2b84d2338 100644
> > > --- a/include/asm-generic/rwonce.h
> > > +++ b/include/asm-generic/rwonce.h
> > > @@ -79,11 +79,14 @@ unsigned long __read_once_word_nocheck(const void *addr)
> > >       (typeof(x))__read_once_word_nocheck(&(x));                      \
> > >  })
> > >
> > > -static __no_kasan_or_inline
> > > +static __no_sanitize_or_inline
> > >  unsigned long read_word_at_a_time(const void *addr)
> > >  {
> > > +     /* open-coded instrument_read(addr, 1) */
> > >       kasan_check_read(addr, 1);
> > > -     return *(unsigned long *)addr;
> > > +     kcsan_check_read(addr, 1);
> > > +
> > > +     return READ_ONCE(*(unsigned long *)addr);
> >
> > I bisected a boot hang that I see on arm64 with LTO enabled to this
> > change as commit ece69af2ede1 ("rwonce: handle KCSAN like KASAN in
> > read_word_at_a_time()") in -next. With LTO, READ_ONCE() gets upgraded to
> > ldar / ldapr, which requires an aligned address to access, but
> > read_word_at_a_time() can be called with an unaligned address. I
> > confirmed this should be the root cause by removing the READ_ONCE()
> > added above or removing the selects of DCACHE_WORD_ACCESS and
> > HAVE_EFFICIENT_UNALIGNED_ACCESS in arch/arm64/Kconfig, which avoids
> > the crash.
>
> Oh, bleeeh. Thanks for figuring that out. I guess that means we should
> remove that READ_ONCE() again to un-break the build. I'll send a patch
> in a bit...

I sent a patch at
<https://lore.kernel.org/all/20250326-rwaat-fix-v1-1-600f411eaf23@google.com/>.

Re: [PATCH] rwonce: handle KCSAN like KASAN in read_word_at_a_time()

Posted by Arnd Bergmann 8 months, 3 weeks ago

On Tue, Mar 25, 2025, at 17:01, Jann Horn wrote:
> Fixes: dfd402a4c4ba ("kcsan: Add Kernel Concurrency Sanitizer infrastructure")
> Signed-off-by: Jann Horn <jannh@google.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

> ---
> This is a low-priority fix. I've never actually hit this issue with
> upstream KCSAN.
> (I only noticed it because I... err... hooked up KASAN to the KCSAN
> hooks. Long story.)
>
> I'm not sure if this should go through Arnd's tree (because it's in
> rwonce.h) or Marco's (because it's a KCSAN thing).
> Going through Marco's tree (after getting an Ack from Arnd) might
> work a little better for me, I may or may not have more KCSAN patches
> in the future.

I agree it's easier if Marco takes it through his tree, as this
is something I rarely touch.

If Marco has nothing else pending for 6.15, I can take it though.

       Arnd

Re: [PATCH] rwonce: handle KCSAN like KASAN in read_word_at_a_time()

Posted by Marco Elver 8 months, 3 weeks ago

On Tue, 25 Mar 2025 at 17:06, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Mar 25, 2025, at 17:01, Jann Horn wrote:
> > Fixes: dfd402a4c4ba ("kcsan: Add Kernel Concurrency Sanitizer infrastructure")
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Marco Elver <elver@google.com>

> > ---
> > This is a low-priority fix. I've never actually hit this issue with
> > upstream KCSAN.
> > (I only noticed it because I... err... hooked up KASAN to the KCSAN
> > hooks. Long story.)

Sounds exciting... ;-)

> > I'm not sure if this should go through Arnd's tree (because it's in
> > rwonce.h) or Marco's (because it's a KCSAN thing).
> > Going through Marco's tree (after getting an Ack from Arnd) might
> > work a little better for me, I may or may not have more KCSAN patches
> > in the future.
>
> I agree it's easier if Marco takes it through his tree, as this
> is something I rarely touch.
>
> If Marco has nothing else pending for 6.15, I can take it though.

I have nothing pending yet. Unless you're very certain there'll be
more KCSAN patches, I'd suggest that Arnd can take it. I'm fine with
KCSAN-related patches that aren't strongly dependent on each other
outside kernel/kcsan to go through whichever tree is closest.

Thanks,
-- Marco

Re: [PATCH] rwonce: handle KCSAN like KASAN in read_word_at_a_time()

Posted by Jann Horn 8 months, 3 weeks ago

On Tue, Mar 25, 2025 at 5:31 PM Marco Elver <elver@google.com> wrote:
> On Tue, 25 Mar 2025 at 17:06, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tue, Mar 25, 2025, at 17:01, Jann Horn wrote:
> > > Fixes: dfd402a4c4ba ("kcsan: Add Kernel Concurrency Sanitizer infrastructure")
> > > Signed-off-by: Jann Horn <jannh@google.com>
[...]
> I have nothing pending yet. Unless you're very certain there'll be
> more KCSAN patches,

No, I don't know yet whether I'll have more KCSAN patches for 6.15.

> I'd suggest that Arnd can take it. I'm fine with
> KCSAN-related patches that aren't strongly dependent on each other
> outside kernel/kcsan to go through whichever tree is closest.

Sounds good to me.