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

[Kees Cook <keescook@chromium.org>]

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