Received-SPF: pass (google.com: domain of linux-kernel+bounces-177966-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 13 May 2024 11:34:20 -0700
From: Kees Cook <keescook@chromium.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Martin Uecker <uecker@tugraz.at>, Justin Stitt <justinstitt@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev
Subject: Re: [RFC] Mitigating unexpected arithmetic overflow
Message-ID: <202405131047.A3861EC13@keescook>
References: <202404291502.612E0A10@keescook>
 <CAHk-=wi5YPwWA8f5RAf_Hi8iL0NhGJeL6MN6UFWwRMY8L6UDvQ@mail.gmail.com>
 <202405081144.D5FCC44A@keescook>
 <CAHk-=wjeiGb1UxCy6Q8aif50C=wWDX9Pgp+WbZYrO72+B1f_QA@mail.gmail.com>
 <202405081354.B0A8194B3C@keescook>
 <CAHk-=wgoE5EkH+sQwi4KhRhCZizUxwZAnC=+9RbZcw7g6016LQ@mail.gmail.com>
 <59f731ab619673afec4956fce6832a1cd5324fb8.camel@tugraz.at>
 <CAHk-=wjERv03yFU7-RUuqX1y89DYHcpdsuu++ako2nR41-EjYg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wjERv03yFU7-RUuqX1y89DYHcpdsuu++ako2nR41-EjYg@mail.gmail.com>

On Sun, May 12, 2024 at 09:09:08AM -0700, Linus Torvalds wrote:
>         unsigned char *p;
>         u32 val;
> 
>         p[0] = val;
>         p[1] = val >> 8;
>         p[2] = val >> 16;
>         p[3] = val >> 24;
> 
> kind of code is both traditional and correct, but obviously drops bits
> very much intentionally on each of those assignments.

The good news here is that the integer implicit truncation sanitizers
are already split between "signed" and "unsigned". So the 2 cases of
exploitable flaws mentioned earlier:

	u8 num_elems;
	...
	num_elems++;		/* int promotion stored back to u8 */

and

	int size;
	u16 read_size;
	...
	read_size = size;	/* large int stored to u16 */

are both confusions across signed/unsigned types, which the signed
sanitizer would catch. The signed sanitizer would entirely ignore
the quoted example at the top: everything is unsigned and no int
promotion is happening.

So, I think we can start with just the "signed integer implicit
truncation" sanitizer. The compiler will still need to deal with the
issues I outlined in [1], where I think we need some consideration
specifically on how to handle things like this (that have a
smaller-than-int size and trip the sanitizer due to int promotion):

u8 checksum(const u8 *buf)
{
	u8 sum = 0;

	for (int i = 0; i < 4; i++)
		sum += buf[i];		/* int promotion */
	return sum;
}

We want "sum" to wrap. We could avoid the "implicit" truncation by
explicitly truncating with something eye-bleedingly horrible like:

		sum = (u8)(sum + buf[i]);

Adding a wrapper for the calculation could work but repeats "sum", and
needs to be explicitly typed, making it just as unfriendly:

		sum = truncate(u8, sum + buf[i]);

Part of the work I'd done in preparation for all this was making the
verbosely named wrapping_assign_add() helper which handles all the
types by examining the arguments and avoids repeating the destination
argument. So this would become:

		wrapping_assign_add(sum, buf[i]);

Still not as clean as "+=", but at least more readable than the
alternatives and leaves no question about wrapping intent.

-Kees

[1] https://lore.kernel.org/lkml/202405081949.0565810E46@keescook/

-- 
Kees Cook