Received-SPF: pass (google.com: domain of linux-crypto+bounces-1544-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 22 Jan 2024 19:07:45 -0800
From: Eric Biggers <ebiggers@kernel.org>
To: 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
Message-ID: <20240123030745.GA1097@sol.localdomain>
References: <20240122235208.work.748-kees@kernel.org>
 <20240123002814.1396804-46-keescook@chromium.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240123002814.1396804-46-keescook@chromium.org>

On Mon, Jan 22, 2024 at 04:27:21PM -0800, Kees Cook wrote:
> In an effort to separate intentional arithmetic wrap-around from
> unexpected wrap-around, we need to refactor places that depend on this
> kind of math. One of the most common code patterns of this is:
> 
> 	VAR + value < VAR
> 
> Notably, this is considered "undefined behavior" for signed and pointer
> types, which the kernel works around by using the -fno-strict-overflow
> option in the build[1] (which used to just be -fwrapv). Regardless, we
> want to get the kernel source to the position where we can meaningfully
> instrument arithmetic wrap-around conditions and catch them when they
> are unexpected, regardless of whether they are signed[2], unsigned[3],
> or pointer[4] types.
> 
> Refactor open-coded wrap-around addition test to use add_would_overflow().
> This paves the way to enabling the wrap-around sanitizers in the future.
> 
> Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1]
> Link: https://github.com/KSPP/linux/issues/26 [2]
> Link: https://github.com/KSPP/linux/issues/27 [3]
> Link: https://github.com/KSPP/linux/issues/344 [4]
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Aditya Srivastava <yashsri421@gmail.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  crypto/adiantum.c                   | 2 +-
>  drivers/crypto/amcc/crypto4xx_alg.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/adiantum.c b/crypto/adiantum.c
> index 60f3883b736a..c2f62ca455af 100644
> --- a/crypto/adiantum.c
> +++ b/crypto/adiantum.c
> @@ -190,7 +190,7 @@ static inline void le128_add(le128 *r, const le128 *v1, const le128 *v2)
>  
>  	r->b = cpu_to_le64(x + y);
>  	r->a = cpu_to_le64(le64_to_cpu(v1->a) + le64_to_cpu(v2->a) +
> -			   (x + y < x));
> +			   (add_would_overflow(x, y)));
>  }
>  
>  /* Subtraction in Z/(2^{128}Z) */
> diff --git a/drivers/crypto/amcc/crypto4xx_alg.c b/drivers/crypto/amcc/crypto4xx_alg.c
> index e0af611a95d8..33f73234ddd9 100644
> --- a/drivers/crypto/amcc/crypto4xx_alg.c
> +++ b/drivers/crypto/amcc/crypto4xx_alg.c
> @@ -251,7 +251,7 @@ crypto4xx_ctr_crypt(struct skcipher_request *req, bool encrypt)
>  	 * the whole IV is a counter.  So fallback if the counter is going to
>  	 * overlow.
>  	 */
> -	if (counter + nblks < counter) {
> +	if (add_would_overflow(counter, nblks)) {
>  		SYNC_SKCIPHER_REQUEST_ON_STACK(subreq, ctx->sw_cipher.cipher);
>  		int ret;

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.

Also, what's the motivation for addressing the 'x + y < x' case but not other
cases in the same file?  For example, the le128_add() function which this patch
modifies has two other intentional wraparounds, which this patch doesn't touch.
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.  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?

- Eric