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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <C9D472CD-13E1-460B-B8FF-3DD9447804B3@kernel.org>
Date: Mon, 22 Jan 2024 19:29:42 -0800
From: Kees Cook <kees@...nel.org>
To: Eric Biggers <ebiggers@...nel.org>, Kees Cook <keescook@...omium.org>
CC: linux-hardening@...r.kernel.org, Herbert Xu <herbert@...dor.apana.org.au>,
 "David S. Miller" <davem@...emloft.net>,
 Aditya Srivastava <yashsri421@...il.com>,
 Randy Dunlap <rdunlap@...radead.org>, linux-crypto@...r.kernel.org,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 46/82] crypto: Refactor intentional wrap-around test

On January 22, 2024 7:07:45 PM PST, Eric Biggers <ebiggers@...nel.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-kees@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-keescook@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-keescook@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