Received-SPF: pass (google.com: domain of linux-crypto+bounces-1545-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 22 Jan 2024 19:29:42 -0800
From: Kees Cook <kees@kernel.org>
To: Eric Biggers <ebiggers@kernel.org>, Kees Cook <keescook@chromium.org>
CC: linux-hardening@vger.kernel.org, Herbert Xu <herbert@gondor.apana.org.au>,
 "David S. Miller" <davem@davemloft.net>,
 Aditya Srivastava <yashsri421@gmail.com>,
 Randy Dunlap <rdunlap@infradead.org>, linux-crypto@vger.kernel.org,
 "Gustavo A. R. Silva" <gustavoars@kernel.org>,
 Bill Wendling <morbo@google.com>, Justin Stitt <justinstitt@google.com>,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH 46/82] crypto: Refactor intentional wrap-around test
User-Agent: K-9 Mail for Android
In-Reply-To: <20240123030745.GA1097@sol.localdomain>
References: <20240122235208.work.748-kees@kernel.org> <20240123002814.1396804-46-keescook@chromium.org> <20240123030745.GA1097@sol.localdomain>
Message-ID: <C9D472CD-13E1-460B-B8FF-3DD9447804B3@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable


On January 22, 2024 7:07:45 PM PST, Eric Biggers <ebiggers@kernel=2Eorg> w=
rote:
>Just to double check, you really intend to forbid *unsigned* integer wrap=
around?
>This patch's commit message focuses on signed, and barely mentions unsign=
ed=2E
>The actual code changes in this patch only deals with unsigned=2E

I don't mean to forbid wrap-around; we just need to annotate it=2E I can s=
ee how this commit log didn't do a great job explaining this=2E I hope the =
cover letter is more sensible:
https://lore=2Ekernel=2Eorg/linux-hardening/20240122235208=2Ework=2E748-ke=
es@kernel=2Eorg/

>Also, what's the motivation for addressing the 'x + y < x' case but not o=
ther
>cases in the same file?

It's a code pattern we could find easily=2E It's working from the instance=
s found via Coccinelle earlier in the series:
https://lore=2Ekernel=2Eorg/linux-hardening/20240123002814=2E1396804-5-kee=
scook@chromium=2Eorg/

> For example, the le128_add() function which this patch
>modifies has two other intentional wraparounds, which this patch doesn't =
touch=2E

For dedicated wrapping functions we can mark them with __unsigned_wrap:
https://lore=2Ekernel=2Eorg/linux-hardening/20240123002814=2E1396804-6-kee=
scook@chromium=2Eorg/

>Also, the le128_sub() function just below le128_add() is very similar but=
 does
>wraparound in the other direction=2E  That's 6 cases in 20 lines of code,=
 but this
>patch only addresses 1=2E  And of course, lots of other crypto code relie=
s on
>unsigned wraparounds too, which this patch overlooks=2E =20

Right -- finding these kinds of things is where a lot of time will be spen=
t in the future, I suspect=2E :)

> So I'm a bit confused
>about the point of this patch=2E  If we really wanted to explicitly annot=
ate all
>the intentional wraparounds in a particular piece of code, so that the co=
de 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 step=
s done=2E And then once the sanitizers are in good shape, the fuzzers can g=
rind=2E (I'm trying to add some parallelism to this project; this code patt=
ern was known so I figured we could address it now=2E)

-Kees

--=20
Kees Cook