UBSAN: annotation to skip sanitization in variable that will wrap

UBSAN: annotation to skip sanitization in variable that will wrap

[Breno Leitao]

Hello,

I am seeing some signed-integer-overflow in percpu reference counters.

	UBSAN: signed-integer-overflow in ./arch/arm64/include/asm/atomic_lse.h:204:1
	-9223372036854775808 - 1 cannot be represented in type 's64' (aka 'long long')
	Call trace:

	 handle_overflow
	 __ubsan_handle_sub_overflow
	 percpu_ref_put_many
	 css_put
	 cgroup_sk_free
	 __sk_destruct
	 __sk_free
	 sk_free
	 unix_release_sock
	 unix_release
	 sock_close

This overflow is probably happening in percpu_ref->percpu_ref_data->count.

Looking at the code documentation, it seems that overflows are fine in
per-cpu values. The lib/percpu-refcount.c code comment says:

 * Note that the counter on a particular cpu can (and will) wrap - this
 * is fine, when we go to shutdown the percpu counters will all sum to
 * the correct value

Is there a way to annotate the code to tell UBSAN that this overflow is
expected and it shouldn't be reported?

Thanks
--breno

Re: UBSAN: annotation to skip sanitization in variable that will wrap

[Justin Stitt]

Hi,

On Wed, Aug 14, 2024 at 10:10 AM Breno Leitao <leitao@debian.org> wrote:
>
> Hello,
>
> I am seeing some signed-integer-overflow in percpu reference counters.

it is brave of you to enable this sanitizer :>)

>
>         UBSAN: signed-integer-overflow in ./arch/arm64/include/asm/atomic_lse.h:204:1
>         -9223372036854775808 - 1 cannot be represented in type 's64' (aka 'long long')
>         Call trace:
>
>          handle_overflow
>          __ubsan_handle_sub_overflow
>          percpu_ref_put_many
>          css_put
>          cgroup_sk_free
>          __sk_destruct
>          __sk_free
>          sk_free
>          unix_release_sock
>          unix_release
>          sock_close
>
> This overflow is probably happening in percpu_ref->percpu_ref_data->count.
>
> Looking at the code documentation, it seems that overflows are fine in
> per-cpu values. The lib/percpu-refcount.c code comment says:
>
>  * Note that the counter on a particular cpu can (and will) wrap - this
>  * is fine, when we go to shutdown the percpu counters will all sum to
>  * the correct value
>
> Is there a way to annotate the code to tell UBSAN that this overflow is
> expected and it shouldn't be reported?

Great question.

1) There exists some new-ish macros in overflow.h that perform
wrapping arithmetic without triggering sanitizer splats -- check out
the wrapping_* suite of macros.

2) I have a Clang attribute in the works [1] that would enable you to
annotate expressions or types that are expected to wrap and will
therefore silence arithmetic overflow/truncation sanitizers. If you
think this could help make the kernel better then I'd appreciate a +1
on that PR so it can get some more review from compiler people! Kees
and I have some other Clang features in the works that will allow for
better mitigation strategies for intended overflow in the kernel.

3) Kees can probably chime in with some other methods of getting the
sanitizer to shush -- we've been doing some work together in this
space. Also check out [2]

>
> Thanks
> --breno
>
>

[1]: https://github.com/llvm/llvm-project/pull/86618
[2]: https://lwn.net/Articles/979747/ (Arithmetic overflow mitigation
in the kernel; Jul 1 2024)

Re: UBSAN: annotation to skip sanitization in variable that will wrap

[Kees Cook]

On Wed, Aug 14, 2024 at 02:05:49PM -0700, Justin Stitt wrote:
> Hi,
> 
> On Wed, Aug 14, 2024 at 10:10 AM Breno Leitao <leitao@debian.org> wrote:
> >
> > Hello,
> >
> > I am seeing some signed-integer-overflow in percpu reference counters.
> 
> it is brave of you to enable this sanitizer :>)
> 
> >
> >         UBSAN: signed-integer-overflow in ./arch/arm64/include/asm/atomic_lse.h:204:1
> >         -9223372036854775808 - 1 cannot be represented in type 's64' (aka 'long long')
> >         Call trace:
> >
> >          handle_overflow
> >          __ubsan_handle_sub_overflow
> >          percpu_ref_put_many
> >          css_put
> >          cgroup_sk_free
> >          __sk_destruct
> >          __sk_free
> >          sk_free
> >          unix_release_sock
> >          unix_release
> >          sock_close
> >
> > This overflow is probably happening in percpu_ref->percpu_ref_data->count.
> >
> > Looking at the code documentation, it seems that overflows are fine in
> > per-cpu values. The lib/percpu-refcount.c code comment says:
> >
> >  * Note that the counter on a particular cpu can (and will) wrap - this
> >  * is fine, when we go to shutdown the percpu counters will all sum to
> >  * the correct value
> >
> > Is there a way to annotate the code to tell UBSAN that this overflow is
> > expected and it shouldn't be reported?
> 
> Great question.
> 
> 1) There exists some new-ish macros in overflow.h that perform
> wrapping arithmetic without triggering sanitizer splats -- check out
> the wrapping_* suite of macros.
> 
> 2) I have a Clang attribute in the works [1] that would enable you to
> annotate expressions or types that are expected to wrap and will
> therefore silence arithmetic overflow/truncation sanitizers. If you
> think this could help make the kernel better then I'd appreciate a +1
> on that PR so it can get some more review from compiler people! Kees
> and I have some other Clang features in the works that will allow for
> better mitigation strategies for intended overflow in the kernel.
> 
> 3) Kees can probably chime in with some other methods of getting the
> sanitizer to shush -- we've been doing some work together in this
> space. Also check out [2]

I haven't checked closely yet, but I *think* top 4 patches here[1]
(proposed here[2]) fix the atomics issues. The haven't landed due to
atomics maintainers wanting differing behavior from the compiler that
Justin is still working on (the "wraps" attribute alluded to above[3]).

-Kees

[1] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=dev/v6.8-rc2/signed-overflow-sanitizer
[2] https://lore.kernel.org/linux-hardening/20240424191225.work.780-kees@kernel.org/
[3] https://github.com/llvm/llvm-project/pull/86618

-- 
Kees Cook

Re: UBSAN: annotation to skip sanitization in variable that will wrap

[Breno Leitao]

Hello Justin,

On Wed, Aug 14, 2024 at 02:05:49PM -0700, Justin Stitt wrote:
> > I am seeing some signed-integer-overflow in percpu reference counters.
> 
> it is brave of you to enable this sanitizer :>)

UBSAN has been somehow useful to pick some problems, so, I try to invest
some time understanding what UBSAN, and see how much it can help when
solving "unexpected" and misterious issues, which is something that
challenges me.

> > Is there a way to annotate the code to tell UBSAN that this overflow is
> > expected and it shouldn't be reported?

> Great question.
> 
> 1) There exists some new-ish macros in overflow.h that perform
> wrapping arithmetic without triggering sanitizer splats -- check out
> the wrapping_* suite of macros.

do they work for atomic? I suppose we also need to have them added to
this_cpu_add(), this_cpu_sub() helpers.

> 2) I have a Clang attribute in the works [1] that would enable you to
> annotate expressions or types that are expected to wrap and will
> therefore silence arithmetic overflow/truncation sanitizers. If you
> think this could help make the kernel better then I'd appreciate a +1
> on that PR so it can get some more review from compiler people! Kees
> and I have some other Clang features in the works that will allow for
> better mitigation strategies for intended overflow in the kernel.

Thanks. I've added a +1 there.

Re: UBSAN: annotation to skip sanitization in variable that will wrap

[Jens Axboe]

On 8/15/24 11:58 AM, Breno Leitao wrote:
>> 1) There exists some new-ish macros in overflow.h that perform
>> wrapping arithmetic without triggering sanitizer splats -- check out
>> the wrapping_* suite of macros.
> 
> do they work for atomic? I suppose we also need to have them added to
> this_cpu_add(), this_cpu_sub() helpers.

I don't think so, it's the bias added specifically to the atomic_long_t
that's the issue with the percpu refs.

-- 
Jens Axboe

Re: UBSAN: annotation to skip sanitization in variable that will wrap

[Kees Cook]

On Thu, Aug 15, 2024 at 12:40:12PM -0600, Jens Axboe wrote:
> On 8/15/24 11:58 AM, Breno Leitao wrote:
> >> 1) There exists some new-ish macros in overflow.h that perform
> >> wrapping arithmetic without triggering sanitizer splats -- check out
> >> the wrapping_* suite of macros.
> > 
> > do they work for atomic? I suppose we also need to have them added to
> > this_cpu_add(), this_cpu_sub() helpers.
> 
> I don't think so, it's the bias added specifically to the atomic_long_t
> that's the issue with the percpu refs.

Yeah, the future annotations will be variable attributes, so it should
be much nicer to apply.

-- 
Kees Cook