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 <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Aditya Srivastava <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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
On January 22, 2024 7:07:45 PM PST, Eric Biggers <[email protected]> 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/[email protected]/
>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/[email protected]/
> 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/[email protected]/
>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