[1/4] counter: Adjust final parameter type in function and signal callbacks

With clang's kernel control flow integrity (kCFI, CONFIG_CFI_CLANG),
indirect call targets are validated against the expected function
pointer prototype to make sure the call target is valid to help mitigate
ROP attacks. If they are not identical, there is a failure at run time,
which manifests as either a kernel panic or thread getting killed. A
proposed warning in clang aims to catch these at compile time, which
reveals:

  drivers/counter/counter-chrdev.c:323:34: error: incompatible function pointer types assigning to 'int (*)(struct counter_device *, struct counter_signal *, u32 *)' (aka 'int (*)(struct counter_device *, struct counter_signal *, unsigned int *)') from 'int (*const)(struct counter_device *, struct counter_signal *, enum counter_signal_level *)' [-Werror,-Wincompatible-function-pointer-types-strict]
                  comp_node.comp.signal_u32_read = counter->ops->signal_read;
                                                ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/counter/counter-chrdev.c:337:33: error: incompatible function pointer types assigning to 'int (*)(struct counter_device *, struct counter_count *, u32 *)' (aka 'int (*)(struct counter_device *, struct counter_count *, unsigned int *)') from 'int (*const)(struct counter_device *, struct counter_count *, enum counter_function *)' [-Werror,-Wincompatible-function-pointer-types-strict]
                  comp_node.comp.count_u32_read = counter->ops->function_read;
                                                ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
  2 errors generated.

  drivers/counter/counter-sysfs.c:845:23: error: incompatible function pointer types assigning to 'int (*)(struct counter_device *, struct counter_signal *, u32 *)' (aka 'int (*)(struct counter_device *, struct counter_signal *, unsigned int *)') from 'int (*const)(struct counter_device *, struct counter_signal *, enum counter_signal_level *)' [-Werror,-Wincompatible-function-pointer-types-strict]
          comp.signal_u32_read = counter->ops->signal_read;
                              ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/counter/counter-sysfs.c:958:22: error: incompatible function pointer types assigning to 'int (*)(struct counter_device *, struct counter_count *, u32 *)' (aka 'int (*)(struct counter_device *, struct counter_count *, unsigned int *)') from 'int (*const)(struct counter_device *, struct counter_count *, enum counter_function *)' [-Werror,-Wincompatible-function-pointer-types-strict]
          comp.count_u32_read = counter->ops->function_read;
                              ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/counter/counter-sysfs.c:959:23: error: incompatible function pointer types assigning to 'int (*)(struct counter_device *, struct counter_count *, u32)' (aka 'int (*)(struct counter_device *, struct counter_count *, unsigned int)') from 'int (*const)(struct counter_device *, struct counter_count *, enum counter_function)' [-Werror,-Wincompatible-function-pointer-types-strict]
          comp.count_u32_write = counter->ops->function_write;
                              ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  3 errors generated.

The ->signal_u32_read(), ->count_u32_read(), and ->count_u32_write()
callbacks in 'struct counter_comp' expect the final parameter to have a
type of 'u32' or 'u32 *' but the ops functions that are being assigned
to those callbacks have an enumerated type as the final parameter. While
these are compatible from an ABI perspective, they will fail the
aforementioned CFI checks.

Adjust the type of the final parameter in the ->signal_read(),
->function_read(), and ->function_write() callbacks in 'struct
counter_ops' and their implementations to match the prototypes in
'struct counter_comp' to clear up these warnings and CFI failures.

Link: https://github.com/ClangBuiltLinux/linux/issues/1750
Reported-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
Cc: Patrick Havelange <patrick.havelange@essensium.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Oleksij Rempel <linux@rempel-privat.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Cc: Julien Panis <jpanis@baylibre.com>
Cc: David Lechner <david@lechnology.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-omap@vger.kernel.org
---
 drivers/counter/104-quad-8.c            | 6 +++---
 drivers/counter/ftm-quaddec.c           | 2 +-
 drivers/counter/intel-qep.c             | 2 +-
 drivers/counter/interrupt-cnt.c         | 4 ++--
 drivers/counter/microchip-tcb-capture.c | 6 +++---
 drivers/counter/stm32-lptimer-cnt.c     | 4 ++--
 drivers/counter/stm32-timer-cnt.c       | 4 ++--
 drivers/counter/ti-ecap-capture.c       | 2 +-
 drivers/counter/ti-eqep.c               | 4 ++--
 include/linux/counter.h                 | 6 +++---
 10 files changed, 20 insertions(+), 20 deletions(-)


base-commit: d501d37841d3b7f18402d71a9ef057eb9dde127e

On Wed, Nov 02, 2022 at 01:23:51PM -0700, Nathan Chancellor wrote:
> On Wed, Nov 02, 2022 at 12:21:23PM -0700, Kees Cook wrote:
> > On Wed, Nov 02, 2022 at 10:22:14AM -0700, Nathan Chancellor wrote:
> > > The ->signal_u32_read(), ->count_u32_read(), and ->count_u32_write()
> > > callbacks in 'struct counter_comp' expect the final parameter to have a
> > > type of 'u32' or 'u32 *' but the ops functions that are being assigned
> > > to those callbacks have an enumerated type as the final parameter. While
> > > these are compatible from an ABI perspective, they will fail the
> > > aforementioned CFI checks.
> > > 
> > > Adjust the type of the final parameter in the ->signal_read(),
> > > ->function_read(), and ->function_write() callbacks in 'struct
> > > counter_ops' and their implementations to match the prototypes in
> > > 'struct counter_comp' to clear up these warnings and CFI failures.
> > 
> > I don't understand these changes. Where do 'struct counter_comp'
> > and 'struct counter_ops' get confused? I can only find matching
> > ops/assignments/calls, so I must be missing something. This looks like
> > a loss of CFI granularity instead of having wrappers added if there is
> > an enum/u32 conversion needed somewhere.
> 
> Right, I am not the biggest fan of this change myself and it is entirely
> possible that I am misreading the warnings from the commit message but I
> do not see how
> 
>         comp_node.comp.signal_u32_read = counter->ops->signal_read;
> 
> and
> 
>         comp_node.comp.count_u32_read = counter->ops->function_read;
> 
> in counter_add_watch(),
> 
>         comp.signal_u32_read = counter->ops->signal_read;
> 
> in counter_signal_attrs_create(), and
> 
>         comp.count_u32_read = counter->ops->function_read;
>         comp.count_u32_write = counter->ops->function_write;
> 
> in counter_count_attrs_create() are currently safe under kCFI, since the
> final parameter type of the prototypes in 'struct counter_ops' does not
> match the final parameter type of the prototypes in 'struct
> counter_comp'. I would expect the indirect calls in counter_get_data()
> and counter_comp_u32_show() to fail currently.
> 
> I briefly looked at making the 'struct counter_comp' callbacks match the
> 'struct counter_ops' ones but the COUNTER_COMP macros in
> include/linux/counter.h made it seem like these callbacks might be used
> by implementations that might use different enumerated types as the
> final parameter. I can look a little closer to see if we can make
> everything match.
> 
> I am not sure how wrappers would work here, I can take a look into how
> feasible that is.

How about this? (I only did signal_read -- similar changes are needed
for function_read and function_write:

diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
index 80acdf62794a..cb391b2498a6 100644
--- a/drivers/counter/counter-chrdev.c
+++ b/drivers/counter/counter-chrdev.c
@@ -38,6 +38,7 @@ struct counter_comp_node {
 	a.device_u32_read == b.device_u32_read || \
 	a.count_u32_read == b.count_u32_read || \
 	a.signal_u32_read == b.signal_u32_read || \
+	a.signal_read == b.signal_read || \
 	a.device_u64_read == b.device_u64_read || \
 	a.count_u64_read == b.count_u64_read || \
 	a.signal_u64_read == b.signal_u64_read || \
@@ -54,6 +55,7 @@ struct counter_comp_node {
 	comp.device_u32_read || \
 	comp.count_u32_read || \
 	comp.signal_u32_read || \
+	comp.signal_read || \
 	comp.device_u64_read || \
 	comp.count_u64_read || \
 	comp.signal_u64_read || \
@@ -320,7 +322,7 @@ static int counter_add_watch(struct counter_device *const counter,
 			return -EINVAL;
 
 		comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
-		comp_node.comp.signal_u32_read = counter->ops->signal_read;
+		comp_node.comp.signal_read = counter->ops->signal_read;
 		break;
 	case COUNTER_COMPONENT_COUNT:
 		if (watch.component.scope != COUNTER_SCOPE_COUNT)
@@ -530,6 +532,7 @@ static int counter_get_data(struct counter_device *const counter,
 	const size_t id = comp_node->component.id;
 	struct counter_signal *const signal = comp_node->parent;
 	struct counter_count *const count = comp_node->parent;
+	enum counter_signal_level level = 0;
 	u8 value_u8 = 0;
 	u32 value_u32 = 0;
 	const struct counter_comp *ext;
@@ -569,8 +572,8 @@ static int counter_get_data(struct counter_device *const counter,
 			ret = comp->device_u32_read(counter, &value_u32);
 			break;
 		case COUNTER_SCOPE_SIGNAL:
-			ret = comp->signal_u32_read(counter, signal,
-						    &value_u32);
+			ret = comp->signal_read(counter, signal, &level);
+			value_u32 = level;
 			break;
 		case COUNTER_SCOPE_COUNT:
 			ret = comp->count_u32_read(counter, count, &value_u32);
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index b9efe66f9f8d..07ce2543b70d 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
@@ -170,6 +170,7 @@ static ssize_t counter_comp_u32_show(struct device *dev,
 	const struct counter_attribute *const a = to_counter_attribute(attr);
 	struct counter_device *const counter = counter_from_dev(dev);
 	const struct counter_available *const avail = a->comp.priv;
+	enum counter_signal_level level = 0;
 	int err;
 	u32 data = 0;
 
@@ -178,7 +179,8 @@ static ssize_t counter_comp_u32_show(struct device *dev,
 		err = a->comp.device_u32_read(counter, &data);
 		break;
 	case COUNTER_SCOPE_SIGNAL:
-		err = a->comp.signal_u32_read(counter, a->parent, &data);
+		err = a->comp.signal_read(counter, a->parent, &level);
+		data = level;
 		break;
 	case COUNTER_SCOPE_COUNT:
 		if (a->comp.type == COUNTER_COMP_SYNAPSE_ACTION)
@@ -842,7 +844,7 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
 
 	/* Create main Signal attribute */
 	comp = counter_signal_comp;
-	comp.signal_u32_read = counter->ops->signal_read;
+	comp.signal_read = counter->ops->signal_read;
 	err = counter_attr_create(dev, cattr_group, &comp, scope, signal);
 	if (err < 0)
 		return err;
diff --git a/include/linux/counter.h b/include/linux/counter.h
index c41fa602ed28..3f1516076f20 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -169,6 +169,9 @@ struct counter_comp {
 				      struct counter_count *count, u32 *val);
 		int (*signal_u32_read)(struct counter_device *counter,
 				       struct counter_signal *signal, u32 *val);
+		int (*signal_read)(struct counter_device *counter,
+				   struct counter_signal *signal,
+				   enum counter_signal_level *level);
 		int (*device_u64_read)(struct counter_device *counter,
 				       u64 *val);
 		int (*count_u64_read)(struct counter_device *counter,