Re: [PATCH 10/32] wcn36xx: Use mem_to_flex_dup() with struct wcn36xx_hal_ind_msg

[Kees Cook <keescook@chromium.org>]

On Wed, May 04, 2022 at 08:42:46AM +0300, Kalle Valo wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
> > As part of the work to perform bounds checking on all memcpy() uses,
> > replace the open-coded a deserialization of bytes out of memory into a
> > trailing flexible array by using a flex_array.h helper to perform the
> > allocation, bounds checking, and copying.
> >
> > Cc: Loic Poulain <loic.poulain@linaro.org>
> > Cc: Kalle Valo <kvalo@kernel.org>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Jakub Kicinski <kuba@kernel.org>
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Cc: wcn36xx@lists.infradead.org
> > Cc: linux-wireless@vger.kernel.org
> > Cc: netdev@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> [...]
> 
> > --- a/drivers/net/wireless/ath/wcn36xx/smd.h
> > +++ b/drivers/net/wireless/ath/wcn36xx/smd.h
> > @@ -46,8 +46,8 @@ struct wcn36xx_fw_msg_status_rsp {
> >  
> >  struct wcn36xx_hal_ind_msg {
> >  	struct list_head list;
> > -	size_t msg_len;
> > -	u8 msg[];
> > +	DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, msg_len);
> > +	DECLARE_FLEX_ARRAY_ELEMENTS(u8, msg);
> 
> This affects readability quite a lot and tbh I don't like it. Isn't
> there any simpler way to solve this?

Similar to how I plumbed member names into __mem_to_flex(), I could do
the same for __mem_to_flex_dup(). That way if the struct member aliases
(DECLARE_FLEX...)  aren't added, the longer form of the helper could
be used. Instead of:

	if (mem_to_flex_dup(&msg_ind, buf, len, GFP_ATOMIC)) {

it would be:

	if (__mem_to_flex_dup(&msg_ind, /* self */, msg,
			      msg_len, buf, len, GFP_ATOMIC)) {

This was how I'd written the helpers in an earlier version, but it
seemed much cleaner to avoid repeating structure layout details at each
call site.

I couldn't find any other way to encode the needed information. It'd be
wonderful if C would let us do:

	struct wcn36xx_hal_ind_msg {
		struct list_head list;
		size_t msg_len;
		u8 msg[msg_len];
	}

And provide some kind of interrogation:

	__builtin_flex_array_member(msg_ind) -> msg_ind->msg
	__builtin_flex_array_count(msg_ind)  -> msg_ind->msg_len

My hope would be to actually use the member aliases to teach things like
-fsanitize=array-bounds about flexible arrays. If it encounters a
structure with the aliases, it could add the instrumentation to do the
bounds checking of things like:

	msg_ind->msg[42]; /* check that 42 is < msg_ind->msg_len */

I also wish I could find a way to make the proposed macros "forward
portable" into proposed C syntax above, but this eluded me as well.
For example:

	struct wcn36xx_hal_ind_msg {
		size_t msg_len;
		struct list_head list;
		BOUNDED_FLEX_ARRAY(u8, msg, msg_len);
	}

	#ifdef CC_HAS_DYNAMIC_ARRAY_LEN
	# define BOUNDED_FLEX_ARRAY(type, name, bounds)	type name[bounds]
	#else
	# define BOUNDED_FLEX_ARRAY(type, name, bounds)			\
		magic_alias_of msg_len __flex_array_elements_count;	\
		union {							\
			type name[];					\
			type __flex_array_elements[];			\
		}
	#endif

But I couldn't sort out the "magic_alias_of" syntax that wouldn't force
structures into having the count member immediately before the flex
array, which would impose more limitations on where this could be
used...

Anyway, I'm open to ideas on how to improve this!

-Kees

-- 
Kees Cook