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:
signature.asc
Description: PGP signature
