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

Commit Message

Kees Cook May 4, 2022, 1:44 a.m. UTC

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 <[e-mail]>
Cc: Kalle Valo <[e-mail]>
Cc: "David S. Miller" <[e-mail]>
Cc: Eric Dumazet <[e-mail]>
Cc: Jakub Kicinski <[e-mail]>
Cc: Paolo Abeni <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Kees Cook <[e-mail]>
---
 drivers/net/wireless/ath/wcn36xx/smd.c | 8 ++------
 drivers/net/wireless/ath/wcn36xx/smd.h | 4 ++--
 2 files changed, 4 insertions(+), 8 deletions(-)

Comments

Kalle Valo May 4, 2022, 5:42 a.m. UTC | #1

Kees Cook <[e-mail]> 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 <[e-mail]>
> Cc: Kalle Valo <[e-mail]>
> Cc: "David S. Miller" <[e-mail]>
> Cc: Eric Dumazet <[e-mail]>
> Cc: Jakub Kicinski <[e-mail]>
> Cc: Paolo Abeni <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Kees Cook <[e-mail]>

[...]

> --- 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?

Kees Cook May 4, 2022, 3:08 p.m. UTC | #2

On Wed, May 04, 2022 at 08:42:46AM +0300, Kalle Valo wrote:
> Kees Cook <[e-mail]> 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 <[e-mail]>
> > Cc: Kalle Valo <[e-mail]>
> > Cc: "David S. Miller" <[e-mail]>
> > Cc: Eric Dumazet <[e-mail]>
> > Cc: Jakub Kicinski <[e-mail]>
> > Cc: Paolo Abeni <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Signed-off-by: Kees Cook <[e-mail]>
> 
> [...]
> 
> > --- 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

Patch

diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index dc3805609284..106af0a2ffc4 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -3343,7 +3343,7 @@  int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
 	const struct wcn36xx_hal_msg_header *msg_header = buf;
 	struct ieee80211_hw *hw = priv;
 	struct wcn36xx *wcn = hw->priv;
-	struct wcn36xx_hal_ind_msg *msg_ind;
+	struct wcn36xx_hal_ind_msg *msg_ind = NULL;
 	wcn36xx_dbg_dump(WCN36XX_DBG_SMD_DUMP, "SMD <<< ", buf, len);
 
 	switch (msg_header->msg_type) {
@@ -3407,16 +3407,12 @@  int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
 	case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
 	case WCN36XX_HAL_PRINT_REG_INFO_IND:
 	case WCN36XX_HAL_SCAN_OFFLOAD_IND:
-		msg_ind = kmalloc(struct_size(msg_ind, msg, len), GFP_ATOMIC);
-		if (!msg_ind) {
+		if (mem_to_flex_dup(&msg_ind, buf, len, GFP_ATOMIC)) {
 			wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
 				    msg_header->msg_type);
 			return -ENOMEM;
 		}
 
-		msg_ind->msg_len = len;
-		memcpy(msg_ind->msg, buf, len);
-
 		spin_lock(&wcn->hal_ind_lock);
 		list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
 		queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
index 3fd598ac2a27..76ecac46f36b 100644
--- 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);
 };
 
 struct wcn36xx;