diff mbox series

[RFC] ubsan: disallow bounds checking with gcov on broken gcc

Message ID 20230601151832.3632525-1-arnd@kernel.org (mailing list archive)
State New, archived
Headers show
Series [RFC] ubsan: disallow bounds checking with gcov on broken gcc | expand

Commit Message

Arnd Bergmann June 1, 2023, 3:18 p.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

Combining UBSAN and GCOV in randconfig builds results in a number of
stack frame size warnings, such as:

crypto/twofish_common.c:683:1: error: the frame size of 2040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c:1589:1: error: the frame size of 1696 bytes is larger than 1400 bytes [-Werror=frame-larger-than=]
drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c:754:1: error: the frame size of 1260 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
drivers/staging/media/ipu3/ipu3-css-params.c:1206:1: error: the frame size of 1080 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
drivers/staging/media/rkvdec/rkvdec-vp9.c:1042:1: error: the frame size of 2176 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
drivers/staging/media/rkvdec/rkvdec-vp9.c:995:1: error: the frame size of 1656 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]

I managed to track this down to the -fsanitize=bounds option clashing
with the -fprofile-arcs option, which leads a lot of spilled temporary
variables in generated instrumentation code.

Hopefully this can be addressed in future gcc releases the same way
that clang handles the combination, but for existing compiler releases,
it seems best to disable one of the two flags. This can be done either
globally by just not passing both at the same time, or locally using
the no_sanitize or no_instrument_function attributes in the affected
functions.

Try the simplest approach here, and turn off -fsanitize=bounds on
gcc when GCOV is enabled, leaving the rest of UBSAN working. Doing
this globally also helps avoid inefficient code from the same
problem that did not push the build over the warning limit.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/stable/6b1a0ee6-c78b-4873-bfd5-89798fce9899@kili.mountain/
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110074
Link: https://godbolt.org/z/zvf7YqK5K
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 lib/Kconfig.ubsan | 2 ++
 1 file changed, 2 insertions(+)

Comments

Kees Cook June 1, 2023, 4:14 p.m. UTC | #1 
On Thu, Jun 01, 2023 at 05:18:11PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Combining UBSAN and GCOV in randconfig builds results in a number of
> stack frame size warnings, such as:
> 
> crypto/twofish_common.c:683:1: error: the frame size of 2040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c:1589:1: error: the frame size of 1696 bytes is larger than 1400 bytes [-Werror=frame-larger-than=]
> drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c:754:1: error: the frame size of 1260 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> drivers/staging/media/ipu3/ipu3-css-params.c:1206:1: error: the frame size of 1080 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> drivers/staging/media/rkvdec/rkvdec-vp9.c:1042:1: error: the frame size of 2176 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> drivers/staging/media/rkvdec/rkvdec-vp9.c:995:1: error: the frame size of 1656 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> 
> I managed to track this down to the -fsanitize=bounds option clashing
> with the -fprofile-arcs option, which leads a lot of spilled temporary
> variables in generated instrumentation code.
> 
> Hopefully this can be addressed in future gcc releases the same way
> that clang handles the combination, but for existing compiler releases,
> it seems best to disable one of the two flags. This can be done either
> globally by just not passing both at the same time, or locally using
> the no_sanitize or no_instrument_function attributes in the affected
> functions.
> 
> Try the simplest approach here, and turn off -fsanitize=bounds on
> gcc when GCOV is enabled, leaving the rest of UBSAN working. Doing
> this globally also helps avoid inefficient code from the same
> problem that did not push the build over the warning limit.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/stable/6b1a0ee6-c78b-4873-bfd5-89798fce9899@kili.mountain/
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110074
> Link: https://godbolt.org/z/zvf7YqK5K
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I think more production systems will have CONFIG_UBSAN_BOUNDS enabled
(e.g. Ubuntu has had it enabled for more than a year now) than GCOV,
so I'd prefer we maintain all*config coverage for the more commonly
used config.

> ---
>  lib/Kconfig.ubsan | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index f7cbbad2bb2f4..8f71ff8f27576 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -29,6 +29,8 @@ config UBSAN_TRAP
>  
>  config CC_HAS_UBSAN_BOUNDS_STRICT
>  	def_bool $(cc-option,-fsanitize=bounds-strict)
> +	# work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110074
> +	depends on GCC_VERSION > 140000 || !GCOV_PROFILE_ALL
>  	help
>  	  The -fsanitize=bounds-strict option is only available on GCC,
>  	  but uses the more strict handling of arrays that includes knowledge

Alternatively, how about falling back to -fsanitize=bounds instead, as
that (which has less coverage) wasn't triggering the stack frame
warnings?

i.e. fall back through these:
	-fsanitize=array-bounds (Clang)
	-fsanitize=bounds-strict (!GCOV || bug fixed in GCC)
	-fsanitize=bounds
Arnd Bergmann June 1, 2023, 5:50 p.m. UTC | #2 
On Thu, Jun 1, 2023, at 18:14, Kees Cook wrote:
> On Thu, Jun 01, 2023 at 05:18:11PM +0200, Arnd Bergmann wrote:
>
> I think more production systems will have CONFIG_UBSAN_BOUNDS enabled
> (e.g. Ubuntu has had it enabled for more than a year now) than GCOV,
> so I'd prefer we maintain all*config coverage for the more commonly
> used config.

Fair enough, I can send that as v2, but let's see what the others
think first.

>>  config CC_HAS_UBSAN_BOUNDS_STRICT
>>  	def_bool $(cc-option,-fsanitize=bounds-strict)
>> +	# work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110074
>> +	depends on GCC_VERSION > 140000 || !GCOV_PROFILE_ALL
>>  	help
>>  	  The -fsanitize=bounds-strict option is only available on GCC,
>>  	  but uses the more strict handling of arrays that includes knowledge
>
> Alternatively, how about falling back to -fsanitize=bounds instead, as
> that (which has less coverage) wasn't triggering the stack frame
> warnings?
>
> i.e. fall back through these:
> 	-fsanitize=array-bounds (Clang)
> 	-fsanitize=bounds-strict (!GCOV || bug fixed in GCC)
> 	-fsanitize=bounds

From what I can tell, -fsanitize=bounds has the same problem
as -fsanitize=bounds-strict, so that would not help.

     Arnd
Kees Cook June 1, 2023, 6:28 p.m. UTC | #3 
On Thu, Jun 01, 2023 at 07:50:38PM +0200, Arnd Bergmann wrote:
> On Thu, Jun 1, 2023, at 18:14, Kees Cook wrote:
> > On Thu, Jun 01, 2023 at 05:18:11PM +0200, Arnd Bergmann wrote:
> >
> > I think more production systems will have CONFIG_UBSAN_BOUNDS enabled
> > (e.g. Ubuntu has had it enabled for more than a year now) than GCOV,
> > so I'd prefer we maintain all*config coverage for the more commonly
> > used config.
> 
> Fair enough, I can send that as v2, but let's see what the others
> think first.
> 
> >>  config CC_HAS_UBSAN_BOUNDS_STRICT
> >>  	def_bool $(cc-option,-fsanitize=bounds-strict)
> >> +	# work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110074
> >> +	depends on GCC_VERSION > 140000 || !GCOV_PROFILE_ALL
> >>  	help
> >>  	  The -fsanitize=bounds-strict option is only available on GCC,
> >>  	  but uses the more strict handling of arrays that includes knowledge
> >
> > Alternatively, how about falling back to -fsanitize=bounds instead, as
> > that (which has less coverage) wasn't triggering the stack frame
> > warnings?
> >
> > i.e. fall back through these:
> > 	-fsanitize=array-bounds (Clang)
> > 	-fsanitize=bounds-strict (!GCOV || bug fixed in GCC)
> > 	-fsanitize=bounds
> 
> From what I can tell, -fsanitize=bounds has the same problem
> as -fsanitize=bounds-strict, so that would not help.

Ah, did something change with GCOV? This (bounds vs bounds-strict) is
the only recent change to CONFIG_UBSAN_BOUNDS...
Arnd Bergmann June 1, 2023, 7:03 p.m. UTC | #4 
On Thu, Jun 1, 2023, at 20:28, Kees Cook wrote:
> On Thu, Jun 01, 2023 at 07:50:38PM +0200, Arnd Bergmann wrote:
>> On Thu, Jun 1, 2023, at 18:14, Kees Cook wrote:
>> >
>> > i.e. fall back through these:
>> > 	-fsanitize=array-bounds (Clang)
>> > 	-fsanitize=bounds-strict (!GCOV || bug fixed in GCC)
>> > 	-fsanitize=bounds
>> 
>> From what I can tell, -fsanitize=bounds has the same problem
>> as -fsanitize=bounds-strict, so that would not help.
>
> Ah, did something change with GCOV? This (bounds vs bounds-strict) is
> the only recent change to CONFIG_UBSAN_BOUNDS...

I missed this problem in my usual randconfig builds because I was
building with GCOV disabled due to disk size limitations until
Dan Carpenter pointed out the specific issue.

I suspect it's been there for a longer time.

      Arnd
diff mbox series

Patch

diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index f7cbbad2bb2f4..8f71ff8f27576 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -29,6 +29,8 @@  config UBSAN_TRAP
 
 config CC_HAS_UBSAN_BOUNDS_STRICT
 	def_bool $(cc-option,-fsanitize=bounds-strict)
+	# work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110074
+	depends on GCC_VERSION > 140000 || !GCOV_PROFILE_ALL
 	help
 	  The -fsanitize=bounds-strict option is only available on GCC,
 	  but uses the more strict handling of arrays that includes knowledge