On January 22, 2024 7:07:45 PM PST, Eric Biggers <ebigg...@kernel.org> 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-k...@kernel.org/
>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-keesc...@chromium.org/
> 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-keesc...@chromium.org/
>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
