| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <202211021621.34241DC39@keescook>
Date: Wed, 2 Nov 2022 16:22:32 -0700
From: Kees Cook <keescook@...omium.org>
To: Nathan Chancellor <nathan@...nel.org>
Cc: William Breathitt Gray <william.gray@...aro.org>,
linux-iio@...r.kernel.org,
Nick Desaulniers <ndesaulniers@...gle.com>,
Tom Rix <trix@...hat.com>,
Sami Tolvanen <samitolvanen@...gle.com>, llvm@...ts.linux.dev,
linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
Patrick Havelange <patrick.havelange@...ensium.com>,
Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
Oleksij Rempel <linux@...pel-privat.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabrice Gasnier <fabrice.gasnier@...s.st.com>,
Vignesh Raghavendra <vigneshr@...com>,
Julien Panis <jpanis@...libre.com>,
David Lechner <david@...hnology.com>,
linux-arm-kernel@...ts.infradead.org,
linux-stm32@...md-mailman.stormreply.com,
linux-omap@...r.kernel.org
Subject: Re: [PATCH 1/4] counter: Adjust final parameter type in function and
signal callbacks
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
Powered by blists - more mailing lists