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

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

[Nathan Chancellor]

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(-)

diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index deed4afadb29..30b40f805f88 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -135,7 +135,7 @@ struct quad8 {
 
 static int quad8_signal_read(struct counter_device *counter,
 			     struct counter_signal *signal,
-			     enum counter_signal_level *level)
+			     u32 *level)
 {
 	const struct quad8 *const priv = counter_priv(counter);
 	unsigned int state;
@@ -258,7 +258,7 @@ static int quad8_function_get(const struct quad8 *const priv, const size_t id,
 
 static int quad8_function_read(struct counter_device *counter,
 			       struct counter_count *count,
-			       enum counter_function *function)
+			       u32 *function)
 {
 	struct quad8 *const priv = counter_priv(counter);
 	unsigned long irqflags;
@@ -275,7 +275,7 @@ static int quad8_function_read(struct counter_device *counter,
 
 static int quad8_function_write(struct counter_device *counter,
 				struct counter_count *count,
-				enum counter_function function)
+				u32 function)
 {
 	struct quad8 *const priv = counter_priv(counter);
 	const int id = count->id;
diff --git a/drivers/counter/ftm-quaddec.c b/drivers/counter/ftm-quaddec.c
index aea6622a9b13..03f03614fc22 100644
--- a/drivers/counter/ftm-quaddec.c
+++ b/drivers/counter/ftm-quaddec.c
@@ -189,7 +189,7 @@ static int ftm_quaddec_count_write(struct counter_device *counter,
 
 static int ftm_quaddec_count_function_read(struct counter_device *counter,
 					   struct counter_count *count,
-					   enum counter_function *function)
+					   u32 *function)
 {
 	*function = COUNTER_FUNCTION_QUADRATURE_X4;
 
diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
index af5942e66f7d..0eedd9e1a94e 100644
--- a/drivers/counter/intel-qep.c
+++ b/drivers/counter/intel-qep.c
@@ -123,7 +123,7 @@ static const enum counter_function intel_qep_count_functions[] = {
 
 static int intel_qep_function_read(struct counter_device *counter,
 				   struct counter_count *count,
-				   enum counter_function *function)
+				   u32 *function)
 {
 	*function = COUNTER_FUNCTION_QUADRATURE_X4;
 
diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 229473855c5b..f068248967d6 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -113,7 +113,7 @@ static const enum counter_function interrupt_cnt_functions[] = {
 
 static int interrupt_cnt_function_read(struct counter_device *counter,
 				       struct counter_count *count,
-				       enum counter_function *function)
+				       u32 *function)
 {
 	*function = COUNTER_FUNCTION_INCREASE;
 
@@ -122,7 +122,7 @@ static int interrupt_cnt_function_read(struct counter_device *counter,
 
 static int interrupt_cnt_signal_read(struct counter_device *counter,
 				     struct counter_signal *signal,
-				     enum counter_signal_level *level)
+				     u32 *level)
 {
 	struct interrupt_cnt_priv *priv = counter_priv(counter);
 	int ret;
diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index e2d1dc6ca668..76bec91fde6c 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -68,7 +68,7 @@ static struct counter_synapse mchp_tc_count_synapses[] = {
 
 static int mchp_tc_count_function_read(struct counter_device *counter,
 				       struct counter_count *count,
-				       enum counter_function *function)
+				       u32 *function)
 {
 	struct mchp_tc_data *const priv = counter_priv(counter);
 
@@ -82,7 +82,7 @@ static int mchp_tc_count_function_read(struct counter_device *counter,
 
 static int mchp_tc_count_function_write(struct counter_device *counter,
 					struct counter_count *count,
-					enum counter_function function)
+					u32 function)
 {
 	struct mchp_tc_data *const priv = counter_priv(counter);
 	u32 bmr, cmr;
@@ -144,7 +144,7 @@ static int mchp_tc_count_function_write(struct counter_device *counter,
 
 static int mchp_tc_count_signal_read(struct counter_device *counter,
 				     struct counter_signal *signal,
-				     enum counter_signal_level *lvl)
+				     u32 *lvl)
 {
 	struct mchp_tc_data *const priv = counter_priv(counter);
 	bool sigstatus;
diff --git a/drivers/counter/stm32-lptimer-cnt.c b/drivers/counter/stm32-lptimer-cnt.c
index d6b80b6dfc28..2dec0c6421d1 100644
--- a/drivers/counter/stm32-lptimer-cnt.c
+++ b/drivers/counter/stm32-lptimer-cnt.c
@@ -155,7 +155,7 @@ static int stm32_lptim_cnt_read(struct counter_device *counter,
 
 static int stm32_lptim_cnt_function_read(struct counter_device *counter,
 					 struct counter_count *count,
-					 enum counter_function *function)
+					 u32 *function)
 {
 	struct stm32_lptim_cnt *const priv = counter_priv(counter);
 
@@ -174,7 +174,7 @@ static int stm32_lptim_cnt_function_read(struct counter_device *counter,
 
 static int stm32_lptim_cnt_function_write(struct counter_device *counter,
 					  struct counter_count *count,
-					  enum counter_function function)
+					  u32 function)
 {
 	struct stm32_lptim_cnt *const priv = counter_priv(counter);
 
diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index 9bf20a5d6bda..ece55113ba85 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -70,7 +70,7 @@ static int stm32_count_write(struct counter_device *counter,
 
 static int stm32_count_function_read(struct counter_device *counter,
 				     struct counter_count *count,
-				     enum counter_function *function)
+				     u32 *function)
 {
 	struct stm32_timer_cnt *const priv = counter_priv(counter);
 	u32 smcr;
@@ -97,7 +97,7 @@ static int stm32_count_function_read(struct counter_device *counter,
 
 static int stm32_count_function_write(struct counter_device *counter,
 				      struct counter_count *count,
-				      enum counter_function function)
+				      u32 function)
 {
 	struct stm32_timer_cnt *const priv = counter_priv(counter);
 	u32 cr1, sms;
diff --git a/drivers/counter/ti-ecap-capture.c b/drivers/counter/ti-ecap-capture.c
index fb1cb1774674..96e5d1f271b8 100644
--- a/drivers/counter/ti-ecap-capture.c
+++ b/drivers/counter/ti-ecap-capture.c
@@ -188,7 +188,7 @@ static int ecap_cnt_count_write(struct counter_device *counter,
 
 static int ecap_cnt_function_read(struct counter_device *counter,
 				  struct counter_count *count,
-				  enum counter_function *function)
+				  u32 *function)
 {
 	*function = COUNTER_FUNCTION_INCREASE;
 
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index b0f24cf3e891..d73a8baa49e8 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -119,7 +119,7 @@ static int ti_eqep_count_write(struct counter_device *counter,
 
 static int ti_eqep_function_read(struct counter_device *counter,
 				 struct counter_count *count,
-				 enum counter_function *function)
+				 u32 *function)
 {
 	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	u32 qdecctl;
@@ -146,7 +146,7 @@ static int ti_eqep_function_read(struct counter_device *counter,
 
 static int ti_eqep_function_write(struct counter_device *counter,
 				  struct counter_count *count,
-				  enum counter_function function)
+				  u32 function)
 {
 	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	enum ti_eqep_count_func qsrc;
diff --git a/include/linux/counter.h b/include/linux/counter.h
index b63746637de2..976dcbfd6266 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -324,17 +324,17 @@ struct counter_event_node {
 struct counter_ops {
 	int (*signal_read)(struct counter_device *counter,
 			   struct counter_signal *signal,
-			   enum counter_signal_level *level);
+			   u32 *level);
 	int (*count_read)(struct counter_device *counter,
 			  struct counter_count *count, u64 *value);
 	int (*count_write)(struct counter_device *counter,
 			   struct counter_count *count, u64 value);
 	int (*function_read)(struct counter_device *counter,
 			     struct counter_count *count,
-			     enum counter_function *function);
+			     u32 *function);
 	int (*function_write)(struct counter_device *counter,
 			      struct counter_count *count,
-			      enum counter_function function);
+			      u32 function);
 	int (*action_read)(struct counter_device *counter,
 			   struct counter_count *count,
 			   struct counter_synapse *synapse,

base-commit: d501d37841d3b7f18402d71a9ef057eb9dde127e
-- 
2.38.1

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

[Nathan Chancellor]

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(-)

diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index deed4afadb29..30b40f805f88 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -135,7 +135,7 @@ struct quad8 {
 
 static int quad8_signal_read(struct counter_device *counter,
 			     struct counter_signal *signal,
-			     enum counter_signal_level *level)
+			     u32 *level)
 {
 	const struct quad8 *const priv = counter_priv(counter);
 	unsigned int state;
@@ -258,7 +258,7 @@ static int quad8_function_get(const struct quad8 *const priv, const size_t id,
 
 static int quad8_function_read(struct counter_device *counter,
 			       struct counter_count *count,
-			       enum counter_function *function)
+			       u32 *function)
 {
 	struct quad8 *const priv = counter_priv(counter);
 	unsigned long irqflags;
@@ -275,7 +275,7 @@ static int quad8_function_read(struct counter_device *counter,
 
 static int quad8_function_write(struct counter_device *counter,
 				struct counter_count *count,
-				enum counter_function function)
+				u32 function)
 {
 	struct quad8 *const priv = counter_priv(counter);
 	const int id = count->id;
diff --git a/drivers/counter/ftm-quaddec.c b/drivers/counter/ftm-quaddec.c
index aea6622a9b13..03f03614fc22 100644
--- a/drivers/counter/ftm-quaddec.c
+++ b/drivers/counter/ftm-quaddec.c
@@ -189,7 +189,7 @@ static int ftm_quaddec_count_write(struct counter_device *counter,
 
 static int ftm_quaddec_count_function_read(struct counter_device *counter,
 					   struct counter_count *count,
-					   enum counter_function *function)
+					   u32 *function)
 {
 	*function = COUNTER_FUNCTION_QUADRATURE_X4;
 
diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
index af5942e66f7d..0eedd9e1a94e 100644
--- a/drivers/counter/intel-qep.c
+++ b/drivers/counter/intel-qep.c
@@ -123,7 +123,7 @@ static const enum counter_function intel_qep_count_functions[] = {
 
 static int intel_qep_function_read(struct counter_device *counter,
 				   struct counter_count *count,
-				   enum counter_function *function)
+				   u32 *function)
 {
 	*function = COUNTER_FUNCTION_QUADRATURE_X4;
 
diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 229473855c5b..f068248967d6 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -113,7 +113,7 @@ static const enum counter_function interrupt_cnt_functions[] = {
 
 static int interrupt_cnt_function_read(struct counter_device *counter,
 				       struct counter_count *count,
-				       enum counter_function *function)
+				       u32 *function)
 {
 	*function = COUNTER_FUNCTION_INCREASE;
 
@@ -122,7 +122,7 @@ static int interrupt_cnt_function_read(struct counter_device *counter,
 
 static int interrupt_cnt_signal_read(struct counter_device *counter,
 				     struct counter_signal *signal,
-				     enum counter_signal_level *level)
+				     u32 *level)
 {
 	struct interrupt_cnt_priv *priv = counter_priv(counter);
 	int ret;
diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index e2d1dc6ca668..76bec91fde6c 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -68,7 +68,7 @@ static struct counter_synapse mchp_tc_count_synapses[] = {
 
 static int mchp_tc_count_function_read(struct counter_device *counter,
 				       struct counter_count *count,
-				       enum counter_function *function)
+				       u32 *function)
 {
 	struct mchp_tc_data *const priv = counter_priv(counter);
 
@@ -82,7 +82,7 @@ static int mchp_tc_count_function_read(struct counter_device *counter,
 
 static int mchp_tc_count_function_write(struct counter_device *counter,
 					struct counter_count *count,
-					enum counter_function function)
+					u32 function)
 {
 	struct mchp_tc_data *const priv = counter_priv(counter);
 	u32 bmr, cmr;
@@ -144,7 +144,7 @@ static int mchp_tc_count_function_write(struct counter_device *counter,
 
 static int mchp_tc_count_signal_read(struct counter_device *counter,
 				     struct counter_signal *signal,
-				     enum counter_signal_level *lvl)
+				     u32 *lvl)
 {
 	struct mchp_tc_data *const priv = counter_priv(counter);
 	bool sigstatus;
diff --git a/drivers/counter/stm32-lptimer-cnt.c b/drivers/counter/stm32-lptimer-cnt.c
index d6b80b6dfc28..2dec0c6421d1 100644
--- a/drivers/counter/stm32-lptimer-cnt.c
+++ b/drivers/counter/stm32-lptimer-cnt.c
@@ -155,7 +155,7 @@ static int stm32_lptim_cnt_read(struct counter_device *counter,
 
 static int stm32_lptim_cnt_function_read(struct counter_device *counter,
 					 struct counter_count *count,
-					 enum counter_function *function)
+					 u32 *function)
 {
 	struct stm32_lptim_cnt *const priv = counter_priv(counter);
 
@@ -174,7 +174,7 @@ static int stm32_lptim_cnt_function_read(struct counter_device *counter,
 
 static int stm32_lptim_cnt_function_write(struct counter_device *counter,
 					  struct counter_count *count,
-					  enum counter_function function)
+					  u32 function)
 {
 	struct stm32_lptim_cnt *const priv = counter_priv(counter);
 
diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index 9bf20a5d6bda..ece55113ba85 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -70,7 +70,7 @@ static int stm32_count_write(struct counter_device *counter,
 
 static int stm32_count_function_read(struct counter_device *counter,
 				     struct counter_count *count,
-				     enum counter_function *function)
+				     u32 *function)
 {
 	struct stm32_timer_cnt *const priv = counter_priv(counter);
 	u32 smcr;
@@ -97,7 +97,7 @@ static int stm32_count_function_read(struct counter_device *counter,
 
 static int stm32_count_function_write(struct counter_device *counter,
 				      struct counter_count *count,
-				      enum counter_function function)
+				      u32 function)
 {
 	struct stm32_timer_cnt *const priv = counter_priv(counter);
 	u32 cr1, sms;
diff --git a/drivers/counter/ti-ecap-capture.c b/drivers/counter/ti-ecap-capture.c
index fb1cb1774674..96e5d1f271b8 100644
--- a/drivers/counter/ti-ecap-capture.c
+++ b/drivers/counter/ti-ecap-capture.c
@@ -188,7 +188,7 @@ static int ecap_cnt_count_write(struct counter_device *counter,
 
 static int ecap_cnt_function_read(struct counter_device *counter,
 				  struct counter_count *count,
-				  enum counter_function *function)
+				  u32 *function)
 {
 	*function = COUNTER_FUNCTION_INCREASE;
 
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index b0f24cf3e891..d73a8baa49e8 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -119,7 +119,7 @@ static int ti_eqep_count_write(struct counter_device *counter,
 
 static int ti_eqep_function_read(struct counter_device *counter,
 				 struct counter_count *count,
-				 enum counter_function *function)
+				 u32 *function)
 {
 	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	u32 qdecctl;
@@ -146,7 +146,7 @@ static int ti_eqep_function_read(struct counter_device *counter,
 
 static int ti_eqep_function_write(struct counter_device *counter,
 				  struct counter_count *count,
-				  enum counter_function function)
+				  u32 function)
 {
 	struct ti_eqep_cnt *priv = ti_eqep_count_from_counter(counter);
 	enum ti_eqep_count_func qsrc;
diff --git a/include/linux/counter.h b/include/linux/counter.h
index b63746637de2..976dcbfd6266 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -324,17 +324,17 @@ struct counter_event_node {
 struct counter_ops {
 	int (*signal_read)(struct counter_device *counter,
 			   struct counter_signal *signal,
-			   enum counter_signal_level *level);
+			   u32 *level);
 	int (*count_read)(struct counter_device *counter,
 			  struct counter_count *count, u64 *value);
 	int (*count_write)(struct counter_device *counter,
 			   struct counter_count *count, u64 value);
 	int (*function_read)(struct counter_device *counter,
 			     struct counter_count *count,
-			     enum counter_function *function);
+			     u32 *function);
 	int (*function_write)(struct counter_device *counter,
 			      struct counter_count *count,
-			      enum counter_function function);
+			      u32 function);
 	int (*action_read)(struct counter_device *counter,
 			   struct counter_count *count,
 			   struct counter_synapse *synapse,

base-commit: d501d37841d3b7f18402d71a9ef057eb9dde127e
-- 
2.38.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] counter: stm32-timer-cnt: Adjust final parameter type of stm32_count_direction_read()

[Nathan Chancellor]

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/stm32-timer-cnt.c:220:2: error: incompatible function pointer types initializing 'int (*)(struct counter_device *, struct counter_count *, u32 *)' (aka 'int (*)(struct counter_device *, struct counter_count *, unsigned int *)') with an expression of type 'int (struct counter_device *, struct counter_count *, enum counter_count_direction *)' [-Werror,-Wincompatible-function-pointer-types-strict]
          COUNTER_COMP_DIRECTION(stm32_count_direction_read),
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./include/linux/counter.h:596:20: note: expanded from macro 'COUNTER_COMP_DIRECTION'
          .count_u32_read = (_read), \
                            ^~~~~~~
  1 error generated

->count_u32_read() in 'struct counter_comp' expects a return type of
'u32 *', not 'enum counter_count_direction *'. Adjust the final
parameter type of stm32_count_direction_read() to match the prototype's
to resolve the warning and CFI failure.

Link: https://github.com/ClangBuiltLinux/linux/issues/1750
Reported-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-stm32@st-md-mailman.stormreply.com
---
 drivers/counter/stm32-timer-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index ece55113ba85..4062296f4bd4 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -137,7 +137,7 @@ static int stm32_count_function_write(struct counter_device *counter,
 
 static int stm32_count_direction_read(struct counter_device *counter,
 				      struct counter_count *count,
-				      enum counter_count_direction *direction)
+				      u32 *direction)
 {
 	struct stm32_timer_cnt *const priv = counter_priv(counter);
 	u32 cr1;
-- 
2.38.1

[PATCH 2/4] counter: stm32-timer-cnt: Adjust final parameter type of stm32_count_direction_read()

[Nathan Chancellor]

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/stm32-timer-cnt.c:220:2: error: incompatible function pointer types initializing 'int (*)(struct counter_device *, struct counter_count *, u32 *)' (aka 'int (*)(struct counter_device *, struct counter_count *, unsigned int *)') with an expression of type 'int (struct counter_device *, struct counter_count *, enum counter_count_direction *)' [-Werror,-Wincompatible-function-pointer-types-strict]
          COUNTER_COMP_DIRECTION(stm32_count_direction_read),
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./include/linux/counter.h:596:20: note: expanded from macro 'COUNTER_COMP_DIRECTION'
          .count_u32_read = (_read), \
                            ^~~~~~~
  1 error generated

->count_u32_read() in 'struct counter_comp' expects a return type of
'u32 *', not 'enum counter_count_direction *'. Adjust the final
parameter type of stm32_count_direction_read() to match the prototype's
to resolve the warning and CFI failure.

Link: https://github.com/ClangBuiltLinux/linux/issues/1750
Reported-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-stm32@st-md-mailman.stormreply.com
---
 drivers/counter/stm32-timer-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index ece55113ba85..4062296f4bd4 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -137,7 +137,7 @@ static int stm32_count_function_write(struct counter_device *counter,
 
 static int stm32_count_direction_read(struct counter_device *counter,
 				      struct counter_count *count,
-				      enum counter_count_direction *direction)
+				      u32 *direction)
 {
 	struct stm32_timer_cnt *const priv = counter_priv(counter);
 	u32 cr1;
-- 
2.38.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] counter: ti-ecap-capture: Adjust final parameter type of ecap_cnt_pol_{read,write}()

[Nathan Chancellor]

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/ti-ecap-capture.c:384:2: error: incompatible function pointer types initializing 'int (*)(struct counter_device *, struct counter_signal *, size_t, u32 *)' (aka 'int (*)(struct counter_device *, struct counter_signal *, unsigned long, unsigned int *)') with an expression of type 'int (struct counter_device *, struct counter_signal *, size_t, enum counter_signal_polarity *)' (aka 'int (struct counter_device *, struct counter_signal *, unsigned long, enum counter_signal_polarity *)') [-Werror,-Wincompatible-function-pointer-types-strict]
          COUNTER_COMP_ARRAY_POLARITY(ecap_cnt_pol_read, ecap_cnt_pol_write, ecap_cnt_pol_array),
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./include/linux/counter.h:627:27: note: expanded from macro 'COUNTER_COMP_ARRAY_POLARITY'
          .signal_array_u32_read = (_read), \
                                  ^~~~~~~
  drivers/counter/ti-ecap-capture.c:384:2: error: incompatible function pointer types initializing 'int (*)(struct counter_device *, struct counter_signal *, size_t, u32)' (aka 'int (*)(struct counter_device *, struct counter_signal *, unsigned long, unsigned int)') with an expression of type 'int (struct counter_device *, struct counter_signal *, size_t, enum counter_signal_polarity)' (aka 'int (struct counter_device *, struct counter_signal *, unsigned long, enum counter_signal_polarity)') [-Werror,-Wincompatible-function-pointer-types-strict]
          COUNTER_COMP_ARRAY_POLARITY(ecap_cnt_pol_read, ecap_cnt_pol_write, ecap_cnt_pol_array),
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./include/linux/counter.h:628:28: note: expanded from macro 'COUNTER_COMP_ARRAY_POLARITY'
          .signal_array_u32_write = (_write), \
                                    ^~~~~~~~
  2 errors generated.

->signal_array_u32_read() and ->signal_array_u32_write() in 'struct
counter_comp' expect a final parameter type of 'u32 *' and 'u32'
respectively, not 'enum counter_signal_polarity *' and 'enum
counter_signal_polarity'. Adjust the final parameter type of
ecap_cnt_pol_{read,write}() to match the prototype's to resolve the
warning and CFI failure.

Link: https://github.com/ClangBuiltLinux/linux/issues/1750
Reported-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Cc: Julien Panis <jpanis@baylibre.com>
Cc: linux-omap@vger.kernel.org
---
 drivers/counter/ti-ecap-capture.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/counter/ti-ecap-capture.c b/drivers/counter/ti-ecap-capture.c
index 96e5d1f271b8..49e349680884 100644
--- a/drivers/counter/ti-ecap-capture.c
+++ b/drivers/counter/ti-ecap-capture.c
@@ -234,7 +234,7 @@ static int ecap_cnt_clk_get_freq(struct counter_device *counter,
 
 static int ecap_cnt_pol_read(struct counter_device *counter,
 			     struct counter_signal *signal,
-			     size_t idx, enum counter_signal_polarity *pol)
+			     size_t idx, u32 *pol)
 {
 	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
 	int bitval;
@@ -250,7 +250,7 @@ static int ecap_cnt_pol_read(struct counter_device *counter,
 
 static int ecap_cnt_pol_write(struct counter_device *counter,
 			      struct counter_signal *signal,
-			      size_t idx, enum counter_signal_polarity pol)
+			      size_t idx, u32 pol)
 {
 	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
 
-- 
2.38.1

[PATCH 4/4] counter: 104-quad-8: Adjust final parameter type of certain callback functions

[Nathan Chancellor]

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/104-quad-8.c:1041:2: error: incompatible function pointer types initializing 'int (*)(struct counter_device *, struct counter_signal *, u32 *)' (aka 'int (*)(struct counter_device *, struct counter_signal *, unsigned int *)') with an expression of type
  'int (struct counter_device *, struct counter_signal *, enum counter_signal_polarity *)' [-Werror,-Wincompatible-function-pointer-types-strict]
          COUNTER_COMP_POLARITY(quad8_polarity_read, quad8_polarity_write,
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./include/linux/counter.h:609:21: note: expanded from macro 'COUNTER_COMP_POLARITY'
          .signal_u32_read = (_read), \
                            ^~~~~~~
  drivers/counter/104-quad-8.c:1041:2: error: incompatible function pointer types initializing 'int (*)(struct counter_device *, struct counter_signal *, u32)' (aka 'int (*)(struct counter_device *, struct counter_signal *, unsigned int)') with an expression of type 'int
  (struct counter_device *, struct counter_signal *, enum counter_signal_polarity)' [-Werror,-Wincompatible-function-pointer-types-strict]
          COUNTER_COMP_POLARITY(quad8_polarity_read, quad8_polarity_write,
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./include/linux/counter.h:610:22: note: expanded from macro 'COUNTER_COMP_POLARITY'
          .signal_u32_write = (_write), \
                              ^~~~~~~~
  drivers/counter/104-quad-8.c:1129:2: error: incompatible function pointer types initializing 'int (*)(struct counter_device *, struct counter_count *, u32 *)' (aka 'int (*)(struct counter_device *, struct counter_count *, unsigned int *)') with an expression of type 'i
  nt (struct counter_device *, struct counter_count *, enum counter_count_mode *)' [-Werror,-Wincompatible-function-pointer-types-strict]
          COUNTER_COMP_COUNT_MODE(quad8_count_mode_read, quad8_count_mode_write,
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./include/linux/counter.h:587:20: note: expanded from macro 'COUNTER_COMP_COUNT_MODE'
          .count_u32_read = (_read), \
                            ^~~~~~~
  drivers/counter/104-quad-8.c:1129:2: error: incompatible function pointer types initializing 'int (*)(struct counter_device *, struct counter_count *, u32)' (aka 'int (*)(struct counter_device *, struct counter_count *, unsigned int)') with an expression of type 'int (
  struct counter_device *, struct counter_count *, enum counter_count_mode)' [-Werror,-Wincompatible-function-pointer-types-strict]
          COUNTER_COMP_COUNT_MODE(quad8_count_mode_read, quad8_count_mode_write,
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./include/linux/counter.h:588:21: note: expanded from macro 'COUNTER_COMP_COUNT_MODE'
          .count_u32_write = (_write), \
                            ^~~~~~~~
  drivers/counter/104-quad-8.c:1131:2: error: incompatible function pointer types initializing 'int (*)(struct counter_device *, struct counter_count *, u32 *)' (aka 'int (*)(struct counter_device *, struct counter_count *, unsigned int *)') with an expression of type 'i
  nt (struct counter_device *, struct counter_count *, enum counter_count_direction *)' [-Werror,-Wincompatible-function-pointer-types-strict]
          COUNTER_COMP_DIRECTION(quad8_direction_read),
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./include/linux/counter.h:596:20: note: expanded from macro 'COUNTER_COMP_DIRECTION'
          .count_u32_read = (_read), \
                            ^~~~~~~
  5 errors generated.

->count_u32_{read,write}() and ->count_u32_{read,write}() in 'struct
counter_comp' expect a final parameter type of either 'u32' or 'u32 *',
not the enumerated types used in the implementations. Adjust the final
parameter type in the implementations to match the prototype's to
resolve the warning 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>
---
 drivers/counter/104-quad-8.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 30b40f805f88..720a88ad59db 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -337,7 +337,7 @@ static int quad8_function_write(struct counter_device *counter,
 
 static int quad8_direction_read(struct counter_device *counter,
 				struct counter_count *count,
-				enum counter_count_direction *direction)
+				u32 *direction)
 {
 	const struct quad8 *const priv = counter_priv(counter);
 	unsigned int ud_flag;
@@ -572,7 +572,7 @@ static int quad8_index_polarity_set(struct counter_device *counter,
 
 static int quad8_polarity_read(struct counter_device *counter,
 			       struct counter_signal *signal,
-			       enum counter_signal_polarity *polarity)
+			       u32 *polarity)
 {
 	int err;
 	u32 index_polarity;
@@ -589,7 +589,7 @@ static int quad8_polarity_read(struct counter_device *counter,
 
 static int quad8_polarity_write(struct counter_device *counter,
 				struct counter_signal *signal,
-				enum counter_signal_polarity polarity)
+				u32 polarity)
 {
 	const u32 pol = (polarity == COUNTER_SIGNAL_POLARITY_POSITIVE) ? 1 : 0;
 
@@ -654,7 +654,7 @@ static int quad8_count_floor_read(struct counter_device *counter,
 
 static int quad8_count_mode_read(struct counter_device *counter,
 				 struct counter_count *count,
-				 enum counter_count_mode *cnt_mode)
+				 u32 *cnt_mode)
 {
 	const struct quad8 *const priv = counter_priv(counter);
 
@@ -679,7 +679,7 @@ static int quad8_count_mode_read(struct counter_device *counter,
 
 static int quad8_count_mode_write(struct counter_device *counter,
 				  struct counter_count *count,
-				  enum counter_count_mode cnt_mode)
+				  u32 cnt_mode)
 {
 	struct quad8 *const priv = counter_priv(counter);
 	unsigned int count_mode;
-- 
2.38.1

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

[Kees Cook]

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.

-- 
Kees Cook

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

[Kees Cook]

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.

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Nathan Chancellor]

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.

Cheers,
Nathan

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

[Nathan Chancellor]

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.

Cheers,
Nathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 3679 bytes --]

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.
> 
> Cheers,
> Nathan

The intention of the code here is to treat the last parameter as an
makeshift generic; the u32 will always be some corresponding enum type
provided by the driver. The expectation is for drivers to define
components via respective COUNTER_COMP_* macros, such that the
assignments of the *_u32_read/*_u32_write callbacks are abstracted away
and the driver can treat the respective last parameter as of the desired
enum type.

For example, COUNTER_COMP_DIRECTION is expected to be used with enum
counter_count_direction, COUNTER_COMP_POLARITY is expected to be used
with enum counter_signal_polarity, etc.

What would be nice is if there is a way to ensure the enum type of the
last parameter of the callback provided to these COUNTER_COMP_* macros
matches the particular respective COUNTER_COMP_* macro's expectation;
e.g. we should get some sort of error if COUNTER_COMP_DIRECTION is used
for a enum counter_signal_level, etc.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 3679 bytes --]

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.
> 
> Cheers,
> Nathan

The intention of the code here is to treat the last parameter as an
makeshift generic; the u32 will always be some corresponding enum type
provided by the driver. The expectation is for drivers to define
components via respective COUNTER_COMP_* macros, such that the
assignments of the *_u32_read/*_u32_write callbacks are abstracted away
and the driver can treat the respective last parameter as of the desired
enum type.

For example, COUNTER_COMP_DIRECTION is expected to be used with enum
counter_count_direction, COUNTER_COMP_POLARITY is expected to be used
with enum counter_signal_polarity, etc.

What would be nice is if there is a way to ensure the enum type of the
last parameter of the callback provided to these COUNTER_COMP_* macros
matches the particular respective COUNTER_COMP_* macro's expectation;
e.g. we should get some sort of error if COUNTER_COMP_DIRECTION is used
for a enum counter_signal_level, etc.

William Breathitt Gray

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Kees Cook]

On Wed, Nov 02, 2022 at 01:23:51PM -0700, Nathan Chancellor wrote:
> 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.

Ah! Thank you -- those were the places I couldn't find.

-- 
Kees Cook

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

[Kees Cook]

On Wed, Nov 02, 2022 at 01:23:51PM -0700, Nathan Chancellor wrote:
> 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.

Ah! Thank you -- those were the places I couldn't find.

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Kees Cook]

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,

-- 
Kees Cook

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

[Kees Cook]

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,

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 8283 bytes --]

On Wed, Nov 02, 2022 at 04:22:32PM -0700, Kees Cook wrote:
> 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:

The reason for the u32 type is that all the Counter enum components can
make use of the same *_u32_read/*_u32_write calls; in other words, we
don't have to handle each Counter enum read/write as unique.

If you want to get rid of that design, then you'll need to create
respective code paths for each Counter enum. See the comments inline
below.

> 
> 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;

This code path is for all Counter enum types currently; changing
signal_u32_read to signal_read here will work for
COUNTER_COMP_SIGNAL_LEVEL but break all the other Counter Signal enum
types. Instead, you should duplicate this code with proper adjustments
for each of the Counter enums in their own respective case blocks.

>  			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;

Same issue as comment above: Counter Signal enums besides
COUNTER_COMP_SIGNAL_LEVEL are possible.

>  		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,
> 
> -- 
> Kees Cook

I'm not familiar with kCFI, but a better solution if possible would be
to hint to the compiler the intention of the code here -- that we're
intentionally type punning the last parameter in order to avoid
duplicate code for each Counter enum because they are all handled the
same way.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 8283 bytes --]

On Wed, Nov 02, 2022 at 04:22:32PM -0700, Kees Cook wrote:
> 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:

The reason for the u32 type is that all the Counter enum components can
make use of the same *_u32_read/*_u32_write calls; in other words, we
don't have to handle each Counter enum read/write as unique.

If you want to get rid of that design, then you'll need to create
respective code paths for each Counter enum. See the comments inline
below.

> 
> 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;

This code path is for all Counter enum types currently; changing
signal_u32_read to signal_read here will work for
COUNTER_COMP_SIGNAL_LEVEL but break all the other Counter Signal enum
types. Instead, you should duplicate this code with proper adjustments
for each of the Counter enums in their own respective case blocks.

>  			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;

Same issue as comment above: Counter Signal enums besides
COUNTER_COMP_SIGNAL_LEVEL are possible.

>  		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,
> 
> -- 
> Kees Cook

I'm not familiar with kCFI, but a better solution if possible would be
to hint to the compiler the intention of the code here -- that we're
intentionally type punning the last parameter in order to avoid
duplicate code for each Counter enum because they are all handled the
same way.

William Breathitt Gray

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel