On Tue, Mar 23, 2021 at 06:54:24PM +0100, Andrew Jones wrote:
> Checking for overflow can be difficult, but doing so may be a good
> idea to avoid difficult to debug problems. Compilers that provide
> builtins for overflow checking allow the checks to be simple
> enough that we can use them more liberally. The idea for this
> flag is to wrap a calculation that should have overflow checking,
> allowing compilers that support it to give us some extra robustness.
> For example,
>
> #ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
> bool overflow = __builtin_mul_overflow(x, y, &z);
> assert(!overflow);
> #else
> /* Older compiler, hopefully we don't overflow... */
> z = x * y;
> #endif
>
> This is a bit ugly though, so when possible we can just use the
> predicate wrappers, which have an always-false fallback, e.g.
>
> /* Old compilers won't assert on overflow. Oh, well... */
> assert(!check_mul_overflow(x, y));
> z = x * y;
>
> Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> ---
>
> v2: Added predicate wrappers
>
> lib/linux/compiler.h | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h
> index 2d72f18c36e5..aa2e3710cf1d 100644
> --- a/lib/linux/compiler.h
> +++ b/lib/linux/compiler.h
> @@ -8,6 +8,39 @@
>
> #ifndef __ASSEMBLY__
>
> +#define GCC_VERSION (__GNUC__ * 10000 \
> + + __GNUC_MINOR__ * 100 \
> + + __GNUC_PATCHLEVEL__)
> +
> +#ifdef __clang__
> +#if __has_builtin(__builtin_add_overflow) && \
> + __has_builtin(__builtin_sub_overflow) && \
> + __has_builtin(__builtin_mul_overflow)
> +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> +#define check_add_overflow(a, b) ({ \
> + typeof((a) + (b)) __d; \
> + __builtin_add_overflow(a, b, &__d); \
> +})
> +#define check_sub_overflow(a, b) ({ \
> + typeof((a) - (b)) __d; \
> + __builtin_sub_overflow(a, b, &__d); \
> +})
> +#define check_mul_overflow(a, b) ({ \
> + typeof((a) * (b)) __d; \
> + __builtin_mul_overflow(a, b, &__d); \
> +})
> +#endif
> +#elif GCC_VERSION >= 50100
> +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> +#define check_add_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) + (b)))0)
> +#define check_sub_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) - (b)))0)
> +#define check_mul_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) * (b)))0)
> +#else
> +#define check_add_overflow(a, b) (0)
> +#define check_sub_overflow(a, b) (0)
> +#define check_mul_overflow(a, b) (0)
> +#endif
> +
> #include <stdint.h>
>
> #define barrier() asm volatile("" : : : "memory")
> --
> 2.26.3
>
I just wanted to point out that with this patch the relevant part of
strtoul becomes
if (is_signed) {
long sacc = (long)acc;
assert(!check_mul_overflow(sacc, base));
assert(!check_add_overflow(sacc * base, c));
} else {
assert(!check_mul_overflow(acc, base));
assert(!check_add_overflow(acc * base, c));
}
acc = acc * base + c;
which looks pretty good to me (if I do say so myself). Unless somebody
shouts I'll queue this patch in arm/queue tomorrow. I'll need to rebase
arm/queue to squash in the fixup to strtoul (I hope nobody thinks that
the arm/queue branch is stable, because it's not!)
I also plan to grab another series from Alexandru, do final testing,
and send Paolo an MR for the whole lot tomorrow.
Thanks,
drew
