From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: Sasha Levin <sashal@kernel.org>,
llvm@lists.linux.dev, stable@vger.kernel.org,
Kees Cook <keescook@chromium.org>,
Rasmus Villemoes <linux@rasmusvillemoes.dk>,
Naresh Kamboju <naresh.kamboju@linaro.org>,
Nathan Chancellor <nathan@kernel.org>,
Stephen Rothwell <sfr@canb.auug.org.au>,
Linus Torvalds <torvalds@linux-foundation.org>,
Pavel Machek <pavel@ucw.cz>
Subject: Re: [PATCH v2] overflow.h: use new generic division helpers to avoid / operator
Date: Tue, 14 Sep 2021 19:22:15 +0200 [thread overview]
Message-ID: <YUDaRwL7h6qJ+7/P@kroah.com> (raw)
In-Reply-To: <20210914002318.2298583-1-ndesaulniers@google.com>
On Mon, Sep 13, 2021 at 05:23:18PM -0700, Nick Desaulniers wrote:
> commit fad7cd3310db ("nbd: add the check to prevent overflow in
> __nbd_ioctl()") raised an issue from the fallback helpers added in
> commit f0907827a8a9 ("compiler.h: enable builtin overflow checkers and
> add fallback code")
>
> ERROR: modpost: "__divdi3" [drivers/block/nbd.ko] undefined!
>
> As Stephen Rothwell notes:
> The added check_mul_overflow() call is being passed 64 bit values.
> COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW is not set for this build (see
> include/linux/overflow.h).
>
> Specifically, the helpers for checking whether the results of a
> multiplication overflowed (__unsigned_mul_overflow,
> __signed_add_overflow) use the division operator when
> !COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW. This is problematic for 64b
> operands on 32b hosts.
>
> This was fixed upstream by
> commit 76ae847497bc ("Documentation: raise minimum supported version of
> GCC to 5.1")
> which is not suitable to be backported to stable; I didn't have this
> patch ready in time.
>
> Cc: stable@vger.kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Suggested-by: Pavel Machek <pavel@ucw.cz>
> Link: https://lore.kernel.org/all/20210909182525.372ee687@canb.auug.org.au/
> Link: https://github.com/ClangBuiltLinux/linux/issues/1438
> Fixes: f0907827a8a9 ("compiler.h: enable builtin overflow checkers and
> add fallback code")
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Changes v1 -> v2:
> * Change the BUILD_BUG_ON_MSG's to check the divisor! Not the dividend!
> * Change __builtin_choose_expr/__builtin_constant_p soup to _Generic as
> per Linus.
> * Add Linus' Suggested-by.
> * use __ prefixes on new macros.
> * add parens around use of macro parameters.
> * realign trailing \.
>
> Note to Rasmus: I did not include comments on the usage. I don't think
> we really intend for folks to be using these, much less so in -stable.
>
> include/linux/math64.h | 37 +++++++++++++++++++++++++++++++++++++
> include/linux/overflow.h | 8 ++++----
> 2 files changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/math64.h b/include/linux/math64.h
> index 66deb1fdc2ef..a1a6ad98b5ea 100644
> --- a/include/linux/math64.h
> +++ b/include/linux/math64.h
> @@ -10,6 +10,9 @@
>
> #define div64_long(x, y) div64_s64((x), (y))
> #define div64_ul(x, y) div64_u64((x), (y))
> +#ifndef is_signed_type
> +#define is_signed_type(type) (((type)(-1)) < (type)1)
> +#endif
>
> /**
> * div_u64_rem - unsigned 64bit divide with 32bit divisor with remainder
> @@ -111,6 +114,15 @@ extern s64 div64_s64(s64 dividend, s64 divisor);
>
> #endif /* BITS_PER_LONG */
>
> +#define __div64_x64(dividend, divisor) ({ \
> + BUILD_BUG_ON_MSG(sizeof(divisor) < sizeof(u64), \
> + "prefer __div_x64"); \
> + __builtin_choose_expr( \
> + is_signed_type(typeof(dividend)), \
> + div64_s64((dividend), (divisor)), \
> + div64_u64((dividend), (divisor))); \
> +})
> +
> /**
> * div_u64 - unsigned 64bit divide with 32bit divisor
> * @dividend: unsigned 64bit dividend
> @@ -141,6 +153,31 @@ static inline s64 div_s64(s64 dividend, s32 divisor)
> }
> #endif
>
> +#define __div_x64(dividend, divisor) ({ \
> + BUILD_BUG_ON_MSG(sizeof(divisor) > sizeof(u32), \
> + "prefer __div64_x64"); \
> + __builtin_choose_expr( \
> + is_signed_type(typeof(dividend)), \
> + div_s64((dividend), (divisor)), \
> + div_u64((dividend), (divisor))); \
> +})
> +
> +#define __div_64(dividend, divisor) \
> + _Generic((divisor), \
> + s64: __div64_x64((dividend), (divisor)), \
> + u64: __div64_x64((dividend), (divisor)), \
> + default: __div_x64((dividend), (divisor)))
> +
> +#define div_64(dividend, divisor) ({ \
> + BUILD_BUG_ON_MSG(sizeof(dividend) > sizeof(u64) || \
> + sizeof(divisor) > sizeof(u64), \
> + "128b div unsupported"); \
> + _Generic((dividend), \
> + s64: __div_64((dividend), (divisor)), \
> + u64: __div_64((dividend), (divisor)), \
> + default: (dividend) / (divisor)); \
> +})
> +
> u32 iter_div_u64_rem(u64 dividend, u32 divisor, u64 *remainder);
>
> #ifndef mul_u32_u32
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index ef74051d5cfe..2ebdf220c184 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -123,8 +123,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> (void) (&__a == __d); \
> *__d = __a * __b; \
> __builtin_constant_p(__b) ? \
> - __b > 0 && __a > type_max(typeof(__a)) / __b : \
> - __a > 0 && __b > type_max(typeof(__b)) / __a; \
> + __b > 0 && __a > div_64(type_max(typeof(__a)), __b) : \
> + __a > 0 && __b > div_64(type_max(typeof(__b)), __a); \
> })
>
> /*
> @@ -195,8 +195,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> (void) (&__a == &__b); \
> (void) (&__a == __d); \
> *__d = (u64)__a * (u64)__b; \
> - (__b > 0 && (__a > __tmax/__b || __a < __tmin/__b)) || \
> - (__b < (typeof(__b))-1 && (__a > __tmin/__b || __a < __tmax/__b)) || \
> + (__b > 0 && (__a > div_64(__tmax, __b) || __a < div_64(__tmin, __b))) || \
> + (__b < (typeof(__b))-1 && (__a > div_64(__tmin, __b) || __a < div_64(__tmax, __b))) || \
> (__b == (typeof(__b))-1 && __a == __tmin); \
> })
>
>
> base-commit: cb83afdc0b865d7c8a74d2b2a1f7dd393e1d196d
> --
> 2.33.0.309.g3052b89438-goog
>
Why is this needed in the 5.10.y tree? I see that commit fad7cd3310db
("nbd: add the check to prevent overflow in __nbd_ioctl()") is planned
to go into 5.14.y and 5.13.y, but no further back at the moment.
confused,
greg k-h
next prev parent reply other threads:[~2021-09-14 17:22 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-13 20:32 [PATCH 5.10] overflow.h: use new generic division helpers to avoid / operator Nick Desaulniers
2021-09-13 21:05 ` Rasmus Villemoes
2021-09-14 0:23 ` [PATCH v2] " Nick Desaulniers
2021-09-14 17:22 ` Greg Kroah-Hartman [this message]
2021-09-14 18:07 ` Nick Desaulniers
2021-09-14 18:12 ` Greg Kroah-Hartman
2021-09-14 17:32 ` [PATCH 5.10] " Kees Cook
2021-09-14 18:14 ` Linus Torvalds
2021-09-14 18:30 ` Linus Torvalds
2021-09-14 18:44 ` Nick Desaulniers
2021-09-14 18:48 ` Linus Torvalds
2021-09-14 18:56 ` Linus Torvalds
2021-09-14 19:10 ` Nick Desaulniers
2021-09-14 19:45 ` Linus Torvalds
2021-09-14 19:50 ` Nick Desaulniers
2021-09-14 19:57 ` Linus Torvalds
2021-09-14 18:47 ` Kees Cook
2021-09-14 18:54 ` Linus Torvalds
2021-09-14 19:12 ` Nick Desaulniers
2021-09-14 19:52 ` Linus Torvalds
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YUDaRwL7h6qJ+7/P@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=keescook@chromium.org \
--cc=linux@rasmusvillemoes.dk \
--cc=llvm@lists.linux.dev \
--cc=naresh.kamboju@linaro.org \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=pavel@ucw.cz \
--cc=sashal@kernel.org \
--cc=sfr@canb.auug.org.au \
--cc=stable@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).