Re: [PATCH 46/82] crypto: Refactor intentional wrap-around test

[Kees Cook]

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