Re: [PATCH] auxdisplay: panel: refactor deprecated strncpy
On Fri, Sep 29, 2023 at 8:42 PM Kees Cook wrote: > > Applied to for-next/hardening, thanks! Thanks for picking it up! Cheers, Miguel
Re: [PATCH 06/82] overflow: Reintroduce signed and unsigned overflow sanitizers
On Tue, Jan 23, 2024 at 1:28 AM Kees Cook wrote:
>
> Because the kernel is built with -fno-strict-overflow, signed and pointer
> arithmetic is defined to always wrap around instead of "overflowing"
> (which would either be elided due to being undefined behavior or would
> wrap around, which led to very weird bugs in the kernel).
By elided I guess you also mean assumed to not happen and thus the
usual chain-of-logic magic?
> So, the config options are added back as CONFIG_UBSAN_SIGNED_WRAP and
> CONFIG_UBSAN_UNSIGNED_WRAP. Since the kernel has several places that
> explicitly depend on wrap-around behavior (e.g. counters, atomics, etc),
> also introduce the __signed_wrap and __unsigned_wrap function attributes
> for annotating functions where wrapping is expected and should not
> be caught. This will allow us to distinguish in the kernel between
> intentional and unintentional cases of arithmetic wrap-around.
Sounds good -- it seems to go in the direction of Rust, i.e. to have a
way to mark expected wrap-arounds so that we can start catching the
unintended ones.
> + depends on !COMPILE_TEST
> + depends on $(cc-option,-fsanitize=signed-integer-overflow)
Maybe this line goes above the other, to be consistent with the
unsigned case? (or the other way around)
> + depends on !X86_32 # avoid excessive stack usage on x86-32/clang
> + depends on !COMPILE_TEST
> + help
> + This option enables -fsanitize=unsigned-integer-overflow which
> checks
> + for wrap-around of any arithmetic operations with unsigned
> integers. This
> + currently causes x86 to fail to boot.
Is it related to the excessive stack usage? In that case, users would
not reach the point to see this description, right? If so, I guess it
could be removed from the `help` and moved into the comment above or
similar.
> +static void test_ubsan_sub_overflow(void)
> +{
> + volatile int val = INT_MIN;
> + volatile unsigned int uval = 0;
> + volatile int val2 = 2;
In the other tests you use a constant instead of `val2`, I am curious
if there is a reason for it?
Thanks!
Cheers,
Miguel
Re: [PATCH 06/82] overflow: Reintroduce signed and unsigned overflow sanitizers
On Tue, Jan 23, 2024 at 5:45 AM Kees Cook wrote: > > Yes. We removed this bad behavior by using -fno-strict-overflow, and we will > want to keep it enabled. Yeah, I only meant that the wording of the commit seems to say there is something special about the "overflowing behavior", i.e. I was expecting just UB with the usual implications, but given the extra text in the parenthesis, I wondered while reading it if there was something different/special going on. > The stack usage is separate. (This may even be fixed in modern Clang; this > comes from the original version of this Kconfig.) The not booting part is > separate and has not been tracked down yet. I see. Thanks! In any case, if the sentence means only 32-bit x86, users couldn't still see it. But since this was already in the revert now that I take a look, I guess ignore this :) > I wondered the same -- they were this way when they were removed, so I just > restored them as they were. :) Makes sense :) Cheers, Miguel
Re: [PATCH] refcount: Annotated intentional signed integer wrap-around
On Wed, Feb 21, 2024 at 6:16 AM Kees Cook wrote: > > Mark the various refcount_t functions with __signed_wrap, as we depend > on the wrapping behavior to detect the overflow and perform saturation. > Silences warnings seen with the LKDTM REFCOUNT_* tests: > > UBSAN: signed-integer-overflow in ../include/linux/refcount.h:189:11 > 2147483647 + 1 cannot be represented in type 'int' > > Signed-off-by: Kees Cook Not sure why I am the "To:" (i.e. even if it is a change involving only an addition of an attribute), but it looks good to me (UBSan is triggering on the few `old + i`s caused by the calls from `drivers/misc/lkdtm/refcount.c`, right?): Reviewed-by: Miguel Ojeda As usual, thanks Kees for keeping up on getting the kernel (un)signed UBSan-clean :) Cheers, Miguel
Re: [PATCH 2/2] rust: time: Use wrapping_sub() for Ktime::sub()
On Wed, Apr 24, 2024 at 1:37 AM Kees Cook wrote:
>
> While working on the signed (and unsigned) integer overflow sanitizer
> support on the C side for the kernel, I've also run into timekeeping
> being a questionable area[1]. I *think* from what I can tell, it's always
> expected to have wrapping behavior.
Thanks, that is useful. In that branch you link, since it is about
unsigned, I imagine you could have hit issues with
`ktime_add_unsafe()` -- after all, callers are expecting overflow
there. But there is a single user (which is `ktime_add_safe()`), and I
don't see that one annotated in your branch.
In any case, if `ktime_add()` is supposed to always be wrapping like
the other functions in the area as you mention, then I think
`ktime_add_unsafe()` (i.e. wrapping one) should be renamed into
`ktime_add()` and the existing one removed. And then we can discuss
whether to do (or not) that in Rust too (see below).
However, if the `ktime_add()` / `ktime_add_unsafe()` /
`ktime_add_safe()` split is there for a good reason (see [1] -- sorry,
you were not in Cc in that particular one), and callers are already
expected to respect it, then I think we should document it in the C
side better and we should just start with that approach in the Rust
side too.
> Can we define the type itself to be wrapping? (This has been my plan on
> the C side, but we're still waiting on a finalized implementation of the
> "wraps" attribute.[2])
Yeah, we can make that the "default", so to speak (i.e. what the
operators will do). But we can also have different methods with
different expectations too if needed (i.e. the usual "access" vs.
"type" discussion).
And given the different variations that exist in C (see [1]), it
seemed to me that `ktime_t` operations there may not be expected to
actually wrap, and thus that would be an argument for trying to be
explicit in Rust.
So for the Rust side, what we need here is the expectation of how
`ktime_t` is supposed to be used. Or, even better, how one would
ideally design `ktime_t` today if there were no worry about callers,
because we have the chance to improve here over the C side before we
have those callers.
If the answer is "everyone assumes those to be wrapping, and there are
almost no use cases where the callers know it is not supposed to wrap,
and we don't care about whether they wrap" etc., then yeah, we should
go with wrapping default semantics and accept that we will not gain
that information and thus not catch mistakes easily in the future.
But if the answer is "we would have liked that `ktime_t` was more
explicit, and that is why we have the `ktime_add{,_safe,_unsafe}()`
variations, but nobody uses them because everybody assumes wrapping"
or "we actually assume no wrapping in `ktime_t`, which is why
`ktime_add_*()` exist", then I think we should definitely try to be
explicit on the Rust side from the onset so that we can catch more
bugs in the future.
(I know you know all this, but I was trying to summarize and clarify
the discussion :)
[1]
https://lore.kernel.org/rust-for-linux/caniq72ka4uvjzb4dn12fpa1wirgdhxcvpurvc7b9t+ipufw...@mail.gmail.com/
Cheers,
Miguel
Re: [PATCH] uapi: stddef.h: Provide UAPI macros for __counted_by_{le, be}
On Mon, May 6, 2024 at 7:42 PM Erick Archer wrote:
>
> Provide UAPI macros for UAPI structs that will gain annotations for
> __counted_by_{le, be} attributes.
>
> Signed-off-by: Erick Archer
I guess this is a mirror of the kernel one at
https://lore.kernel.org/lkml/20240327142241.1745989-1-aleksander.loba...@intel.com/,
but it would be nice to add some context, e.g. Link: tag, to this, and
possibly a comment or two.
Thanks!
Cheers,
Miguel
Re: [PATCH] string: Check for "nonstring" attribute on strscpy() arguments
On Mon, Aug 5, 2024 at 11:43 PM Kees Cook wrote: > > +/* Determine if an attribute has been applied to a variable. */ > +#if __has_builtin(__builtin_has_attribute) > +#define __annotated(var, attr) __builtin_has_attribute(var, attr) > +#else > +#define __annotated(var, attr) (false) > +#endif `__annotated` is obviously best-effort given this definition, and we do similar things elsewhere, and it has a double-underscore. However, I wonder if this being a "query" (vs. something like an attribute) may imply that it has a greater risk of someone thinking it will always reply with the right answer... (if e.g. they copy-paste another use). Perhaps there is a more explicit name to let users recall that. Anyway, it looks sensible to me: more compile-time checking seldomly hurts (apart from complexity in these definitions :). So: Reviewed-by: Miguel Ojeda I also introduced a mistake on purpose and I got the expected build error, so: Tested-by: Miguel Ojeda Cheers, Miguel
Re: [RFC PATCH 01/11] kbuild: ignore .config rule for make --always-make
On Sat, Nov 2, 2024 at 10:08 PM Nicolas Schier wrote:
>
> As we still also support make v3.80 to v4.0, please use $(short-opts)
> defined around line 27.
We moved to 4.0 in 5f99665ee8f4 ("kbuild: raise the minimum GNU Make
requirement to 4.0") -- or do you mean something else / am I missing
something?
Thanks!
Cheers,
Miguel
Re: [PATCH 0/3] string.h: Use ARRAY_SIZE() for memtostr*()/strtomem*()
On Thu, Feb 6, 2025 at 7:42 PM Andy Shevchenko wrote: > > What's the minimum Clang version we build kernel with? 12? 13.0.1 for most architectures according to `scripts/min-tool-version.sh`. Cheers, Miguel
Re: [PATCH 1/3] compiler.h: Move C string helpers into C-only kernel section
On Thu, Feb 6, 2025 at 7:11 PM Kees Cook wrote: > > The C kernel helpers for evaluating C Strings were placed outside of the > assembly ifdef. Move them to the correct place so future changes won't > confuse the assembler. This moves it also into the kernel ifdef too -- I assume that is intentional. (Perhaps mention ifndef instead of ifdef?) I checked that this is indeed a pure move, so: Reviewed-by: Miguel Ojeda Cheers, Miguel
Re: [PATCH] compiler_types: Introduce __nonstring_array
On Mon, Mar 10, 2025 at 10:42 PM Kees Cook wrote: > > GCC has expanded support of the "nonstring" attribute so that it can be > applied to arrays of character arrays[1], which is needed to identify > correct static initialization of those kinds of objects. Since this was > not supported prior to GCC 15, we need to distinguish the usage of Linux's > existing __nonstring macro for the attribute for non-multi-dimensional > char arrays. Until GCC 15 is the minimum version, use __nonstring_array to > mark arrays of non-string character arrays. (Regular non-string character > arrays can continue to use __nonstring.) Sounds reasonable. So this means that, when the GCC minimum goes over 15 in some years, we can both remove it and move it to `compiler_attributes.h`, right? Perhaps we should mention part of this commit message (or the sentence above) in the comment to clarify (it will also help to clarify what that "only supported since GCC >= 15" means). If you are going to use it in a series that has a use case: Acked-by: Miguel Ojeda Thanks! Cheers, Miguel
