From adcd7118caadac54666081be39bbbc3e9b5e1f7d Mon Sep 17 00:00:00 2001 From: Alexey Dobriyan Date: Thu, 20 Oct 2022 12:49:45 +0300 Subject: [PATCH 1/6] perf/x86: Make struct p4_event_bind::cntr signed array struct p4_event_bind::cntr[][] should be signed because of the following code: int i, j; for (i = 0; i < P4_CNTR_LIMIT; i++) { ---> j = bind->cntr[thread][i]; if (j != -1 && !test_bit(j, used_mask)) return j; } Making this member unsigned will make "j" 255 and fail "j != -1" comparison. Signed-off-by: Alexey Dobriyan Signed-off-by: Jason A. Donenfeld --- arch/x86/events/intel/p4.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/events/intel/p4.c b/arch/x86/events/intel/p4.c index 03bbcc2fa2ff..35936188db01 100644 --- a/arch/x86/events/intel/p4.c +++ b/arch/x86/events/intel/p4.c @@ -24,7 +24,7 @@ struct p4_event_bind { unsigned int escr_msr[2]; /* ESCR MSR for this event */ unsigned int escr_emask; /* valid ESCR EventMask bits */ unsigned int shared; /* event is shared across threads */ - char cntr[2][P4_CNTR_LIMIT]; /* counter index (offset), -1 on absence */ + signed char cntr[2][P4_CNTR_LIMIT]; /* counter index (offset), -1 on absence */ }; struct p4_pebs_bind { From 51c9daec79b4fa8140cd5490abec4f1f3685ecd8 Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Fri, 28 Oct 2022 16:22:56 +0200 Subject: [PATCH 2/6] sparc: sbus: treat CPU index as integer Using a `char` to fit a CPU index is too small, and assigning -1 to it isn't correct either, since `char` is sometimes signed and sometimes not. In this case, it's been fine since this driver only works on sparc, but that will soon be changing when chars become unsigned everywhere. So instead, use a normal `int` type, which matches the `int cpu` function argument that this is being compared against. Cc: David S. Miller Signed-off-by: Jason A. Donenfeld --- drivers/sbus/char/envctrl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/sbus/char/envctrl.c b/drivers/sbus/char/envctrl.c index 843e830b5f87..ea914a7eaa7f 100644 --- a/drivers/sbus/char/envctrl.c +++ b/drivers/sbus/char/envctrl.c @@ -363,8 +363,8 @@ static int envctrl_read_cpu_info(int cpu, struct i2c_child_t *pchild, char mon_type, unsigned char *bufdata) { unsigned char data; - int i; - char *tbl, j = -1; + int i, j = -1; + char *tbl; /* Find the right monitor type and channel. */ for (i = 0; i < PCF8584_MAX_CHANNELS; i++) { From 7392134428c92a4cb541bd5c8f4f5c8d2e88364d Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Mon, 24 Oct 2022 17:23:43 +0200 Subject: [PATCH 3/6] media: stv0288: use explicitly signed char With char becoming unsigned by default, and with `char` alone being ambiguous and based on architecture, signed chars need to be marked explicitly as such. Use `s8` and `u8` types here, since that's what surrounding code does. This fixes: drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: assigning (-9) to unsigned variable 'tm' drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: we never enter this loop Cc: Mauro Carvalho Chehab Cc: linux-media@vger.kernel.org Cc: stable@vger.kernel.org Signed-off-by: Jason A. Donenfeld --- drivers/media/dvb-frontends/stv0288.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/media/dvb-frontends/stv0288.c b/drivers/media/dvb-frontends/stv0288.c index 3d54a0ec86af..3ae1f3a2f142 100644 --- a/drivers/media/dvb-frontends/stv0288.c +++ b/drivers/media/dvb-frontends/stv0288.c @@ -440,9 +440,8 @@ static int stv0288_set_frontend(struct dvb_frontend *fe) struct stv0288_state *state = fe->demodulator_priv; struct dtv_frontend_properties *c = &fe->dtv_property_cache; - char tm; - unsigned char tda[3]; - u8 reg, time_out = 0; + u8 tda[3], reg, time_out = 0; + s8 tm; dprintk("%s : FE_SET_FRONTEND\n", __func__); From daf4218bf8dd9750d1ad93846aee98bf2e08eeee Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Thu, 3 Nov 2022 01:15:37 +0100 Subject: [PATCH 4/6] media: atomisp: make hive_int8 explictly signed The current definition of hive_int8 is a naked char, without any sign specifier. This is incorrect on platforms such as arm, where char is unsigned. Fortunately nothing in the kernel actually uses a hive_int8 type, but in case it gets used later rather than removed, this makes it explicitly signed. Cc: Mauro Carvalho Chehab Cc: Sakari Ailus Cc: Greg Kroah-Hartman Signed-off-by: Jason A. Donenfeld --- drivers/staging/media/atomisp/pci/hive_types.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/atomisp/pci/hive_types.h b/drivers/staging/media/atomisp/pci/hive_types.h index 4b8a679fb672..55d36931f079 100644 --- a/drivers/staging/media/atomisp/pci/hive_types.h +++ b/drivers/staging/media/atomisp/pci/hive_types.h @@ -42,7 +42,7 @@ typedef unsigned int hive_bool; #define hive_false 0 #define hive_true 1 -typedef char hive_int8; +typedef signed char hive_int8; typedef short hive_int16; typedef int hive_int32; typedef long long hive_int64; From 3bc753c06dd02a3517c9b498e3846ebfc94ac3ee Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Wed, 19 Oct 2022 10:26:48 -0600 Subject: [PATCH 5/6] kbuild: treat char as always unsigned Recently, some compile-time checking I added to the clamp_t family of functions triggered a build error when a poorly written driver was compiled on ARM, because the driver assumed that the naked `char` type is signed, but ARM treats it as unsigned, and the C standard says it's architecture-dependent. I doubt this particular driver is the only instance in which unsuspecting authors make assumptions about `char` with no `signed` or `unsigned` specifier. We were lucky enough this time that that driver used `clamp_t(char, negative_value, positive_value)`, so the new checking code found it, and I've sent a patch to fix it, but there are likely other places lurking that won't be so easily unearthed. So let's just eliminate this particular variety of heisensign bugs entirely. Set `-funsigned-char` globally, so that gcc makes the type unsigned on all architectures. This will break things in some places and fix things in others, so this will likely cause a bit of churn while reconciling the type misuse. Cc: Masahiro Yamada Cc: Kees Cook Cc: Andrew Morton Cc: Linus Torvalds Cc: Andy Shevchenko Cc: Greg Kroah-Hartman Link: https://lore.kernel.org/lkml/202210190108.ESC3pc3D-lkp@intel.com/ Signed-off-by: Jason A. Donenfeld --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 58cd4f5e1c3a..c329c910c8b0 100644 --- a/Makefile +++ b/Makefile @@ -562,7 +562,7 @@ KBUILD_AFLAGS := -D__ASSEMBLY__ -fno-PIE KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \ -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \ -Werror=implicit-function-declaration -Werror=implicit-int \ - -Werror=return-type -Wno-format-security \ + -Werror=return-type -Wno-format-security -funsigned-char \ -std=gnu11 KBUILD_CPPFLAGS := -D__KERNEL__ KBUILD_RUSTFLAGS := $(rust_common_flags) \ From 0445d1bae1cce00ae4e99c8cde33784a8199bad6 Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Thu, 3 Nov 2022 01:50:05 +0100 Subject: [PATCH 6/6] lib: assume char is unsigned Now that we use -funsigned-char, there's no need for this kind of ifdef. Signed-off-by: Jason A. Donenfeld --- lib/is_signed_type_kunit.c | 4 ---- lib/test_printf.c | 12 ------------ 2 files changed, 16 deletions(-) diff --git a/lib/is_signed_type_kunit.c b/lib/is_signed_type_kunit.c index 207207522925..0a7f6ae62839 100644 --- a/lib/is_signed_type_kunit.c +++ b/lib/is_signed_type_kunit.c @@ -21,11 +21,7 @@ static void is_signed_type_test(struct kunit *test) KUNIT_EXPECT_EQ(test, is_signed_type(bool), false); KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true); KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false); -#ifdef __CHAR_UNSIGNED__ KUNIT_EXPECT_EQ(test, is_signed_type(char), false); -#else - KUNIT_EXPECT_EQ(test, is_signed_type(char), true); -#endif KUNIT_EXPECT_EQ(test, is_signed_type(int), true); KUNIT_EXPECT_EQ(test, is_signed_type(unsigned int), false); KUNIT_EXPECT_EQ(test, is_signed_type(long), true); diff --git a/lib/test_printf.c b/lib/test_printf.c index 4bd15a593fbd..5eb889679e4f 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -179,18 +179,6 @@ test_number(void) * behaviour. */ test("00|0|0|0|0", "%.2d|%.1d|%.0d|%.*d|%1.0d", 0, 0, 0, 0, 0, 0); -#ifndef __CHAR_UNSIGNED__ - { - /* - * Passing a 'char' to a %02x specifier doesn't do - * what was presumably the intention when char is - * signed and the value is negative. One must either & - * with 0xff or cast to u8. - */ - char val = -16; - test("0xfffffff0|0xf0|0xf0", "%#02x|%#02x|%#02x", val, val & 0xff, (u8)val); - } -#endif } static void __init