| Message ID | 20240123002814.1396804-34-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | overflow: Refactor open-coded arithmetic wrap-around | expand |
On Mon, 22 Jan 2024 at 16:46, Kees Cook <keescook@chromium.org> wrote: > > Refactor open-coded unsigned wrap-around addition test to use > check_add_overflow(), NAK. First off, none of this has anything to do with -fno-strict-overflow. We do that, because without it gcc ends up doing various odd and surprising things, the same way it does with strict-aliasing. IOW, you should think of -fno-strict-overflow as a hardening thing. Any optimization that depends on "this can overflow, so I can do anything I want" is just a dangerous optimization for the kernel. It matches -fno-strict-aliasing and -fno-delete-null-pointer-checks, in other words. And I do not understand why you mention it in the first place, since this code USES UNSIGNED INTEGER ARITHMETIC, and thus has absolutely nothing to do with that no-strict-overflow flag. So the commit message is actively misleading and broken. Unsigned arithmetic has very well-defined behavior, and the code uses that with a very traditional and valid test. The comment about "redundant open-coded addition" is also PURE GARBAGE, since the compiler will trivially do the CSE - and on the source code level your modified code is actively bigger and uglier. So your patch improves neither code generation or source code. And if there's some unsigned wrap-around checker that doesn't understand this traditional way of doing overflow checking, that piece of crap needs fixing. I don't want to see mindless conversion patches that work around some broken tooling. I want to see them even less when pretty much EVERY SINGLE WORD in the commit message seems to be actively misleading and irrelevant garbage. Stop making the world a worse place. Linus
On Mon, Jan 22, 2024 at 05:07:40PM -0800, Linus Torvalds wrote: > First off, none of this has anything to do with -fno-strict-overflow. > We do that, because without it gcc ends up doing various odd and > surprising things, the same way it does with strict-aliasing. > > IOW, you should think of -fno-strict-overflow as a hardening thing. > Any optimization that depends on "this can overflow, so I can do > anything I want" is just a dangerous optimization for the kernel. > > It matches -fno-strict-aliasing and -fno-delete-null-pointer-checks, > in other words. > > And I do not understand why you mention it in the first place, since > this code USES UNSIGNED INTEGER ARITHMETIC, and thus has absolutely > nothing to do with that no-strict-overflow flag. I've tried to find the right balance between not enough details and too much. I guess I got it wrong. I go into more detail in the cover letter: https://lore.kernel.org/linux-hardening/20240122235208.work.748-kees@kernel.org/ Basically, this isn't about -fno-strict-overflow -- that's just a wrinkle on the compiler side. I completely agree: we have to keep the "undefined behavior" junk as far away from the kernel as possible. This is about disambiguating the intent of C arithmetic so we can be in the position to instrument the kernel to catch unexpected wrap-arounds. We can't use C++ tricks with operator overloading and the introduction of wrapping vs trapping types, so we have to annotate things directly. And we have sanitizers for signed, unsigned, and pointers. Hence, the need to adjust these open-coded wrap tests. It's not about removing UB: -fno-strict-overflow already does that. This is about let us trap unexpected wrap-around, regardless of type. > Stop making the world a worse place. Rude. I'll leave that to ice storms.
On Mon, 22 Jan 2024 at 17:38, Kees Cook <keescook@chromium.org> wrote: > > I've tried to find the right balance between not enough details and too > much. I guess I got it wrong. My complaint isn't about the level of detail. My complaint is about how the commit log IS ACTIVELY MISLEADING GARBAGE and does not match the actual patch in any way, shape, or form. It talks about completely irrelevant issues that simply have nothing to do with it. It talks about undefined behavior and about a "unsigned wrap-around sanitizer[2]", which is nonsensical, since there is no undefined behavior to sanitize. It literally gives a link to a github "issue" for that claim, but when you follow the link, it's actually about *signed* overflow, which is something entirely different. And honestly, the patch itself is garbage. The code is fine. Any "sanitizer" that complains about that code is pure and utter shite. Really. If you actually have some real "detect unsigned wraparound" tool (NOTE: that is *NOT* undefined behavior, and that is *NOT* a "sanitizer", it's at most some helpful checker), then such a tool had better recognize the perfectly fine traditional idiom for this, which is to do the addition and check that the result is smaller. Like the code does. See what I'm saying? The patch is garbage. Any sanitizer that would complain about the old code is garbage. And the commit message is worse than garbage, it is actively misleading to the point that I'd call it lying, trying to confuse the issues by bringing up things that are utterly and entirely irrelevant to the patch. So: - get rid of that commit message that is lying garbage - fix the so-called "sanitizer". - stop calling the unsigned wrap-around a "sanitizer" and talking about "undefined behavior" in the same sentence, since it's neither. Do you really not see why I think that thing is actively *WRONG*? Linus
On Tue, Jan 23, 2024 at 10:06:12AM -0800, Linus Torvalds wrote: > So: > > - get rid of that commit message that is lying garbage > > - fix the so-called "sanitizer". > > - stop calling the unsigned wrap-around a "sanitizer" and talking > about "undefined behavior" in the same sentence, since it's neither. > > Do you really not see why I think that thing is actively *WRONG*? Yes -- I was trying to head off the confusion about what the larger goal is (trapping unexpected wrap-around) so that people don't assume I'm talking about Undefined Behavior (we don't have any UB arithmetic in the kernel, very intentionally). I did not succeed! I'll rewrite it all.
diff --git a/ipc/shm.c b/ipc/shm.c index a89f001a8bf0..227a1610628a 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -1650,11 +1650,13 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, } if (addr && !(shmflg & SHM_REMAP)) { + unsigned long sum; + err = -EINVAL; - if (addr + size < addr) + if (check_add_overflow(addr, size, &sum)) goto invalid; - if (find_vma_intersection(current->mm, addr, addr + size)) + if (find_vma_intersection(current->mm, addr, sum)) goto invalid; }
In an effort to separate intentional arithmetic wrap-around from unexpected wrap-around, we need to refactor places that depend on this kind of math. One of the most common code patterns of this is: VAR + value < VAR Notably, this is considered "undefined behavior" for signed and pointer types, which the kernel works around by using the -fno-strict-overflow option in the build[1] (which used to just be -fwrapv). Regardless, we want to get the kernel source to the position where we can meaningfully instrument arithmetic wrap-around conditions and catch them when they are unexpected, regardless of whether they are signed[2], unsigned[3], or pointer[4] types. Refactor open-coded unsigned wrap-around addition test to use check_add_overflow(), retaining the result for later usage (which removes the redundant open-coded addition). This paves the way to enabling the unsigned wrap-around sanitizer[2] in the future. Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1] Link: https://github.com/KSPP/linux/issues/26 [2] Link: https://github.com/KSPP/linux/issues/27 [3] Link: https://github.com/KSPP/linux/issues/344 [4] Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: "Liam R. Howlett" <Liam.Howlett@oracle.com> Cc: Mark Brown <broonie@kernel.org> Cc: Mike Kravetz <mike.kravetz@oracle.com> Cc: Vasily Averin <vasily.averin@linux.dev> Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com> Signed-off-by: Kees Cook <keescook@chromium.org> --- ipc/shm.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)