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:

quoted

On Wed, Nov 02, 2022 at 10:22:14AM -0700, Nathan Chancellor wrote:

quoted

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