On January 22, 2024 7:07:45 PM PST, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: >Just to double check, you really intend to forbid *unsigned* integer wraparound? >This patch's commit message focuses on signed, and barely mentions unsigned. >The actual code changes in this patch only deals with unsigned. I don't mean to forbid wrap-around; we just need to annotate it. I can see how this commit log didn't do a great job explaining this. I hope the cover letter is more sensible: https://lore.kernel.org/linux-hardening/20240122235208.work.748-kees@xxxxxxxxxx/ >Also, what's the motivation for addressing the 'x + y < x' case but not other >cases in the same file? It's a code pattern we could find easily. It's working from the instances found via Coccinelle earlier in the series: https://lore.kernel.org/linux-hardening/20240123002814.1396804-5-keescook@xxxxxxxxxxxx/ > For example, the le128_add() function which this patch >modifies has two other intentional wraparounds, which this patch doesn't touch. For dedicated wrapping functions we can mark them with __unsigned_wrap: https://lore.kernel.org/linux-hardening/20240123002814.1396804-6-keescook@xxxxxxxxxxxx/ >Also, the le128_sub() function just below le128_add() is very similar but does >wraparound in the other direction. That's 6 cases in 20 lines of code, but this >patch only addresses 1. And of course, lots of other crypto code relies on >unsigned wraparounds too, which this patch overlooks. Right -- finding these kinds of things is where a lot of time will be spent in the future, I suspect. :) > So I'm a bit confused >about the point of this patch. If we really wanted to explicitly annotate all >the intentional wraparounds in a particular piece of code, so that the code can >be run with the corresponding sanitizer enabled, wouldn't it be necessary to >actually test the code with that sanitizer enabled to find all the cases? Yes, but there's a lot of code to test -- I'm trying to get the first steps done. And then once the sanitizers are in good shape, the fuzzers can grind. (I'm trying to add some parallelism to this project; this code pattern was known so I figured we could address it now.) -Kees -- Kees Cook
