| Message ID | 20220504014440.3697851-11-keescook@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce flexible array struct memcpy() helpers | expand |
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?
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
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;
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> --- drivers/net/wireless/ath/wcn36xx/smd.c | 8 ++------ drivers/net/wireless/ath/wcn36xx/smd.h | 4 ++-- 2 files changed, 4 insertions(+), 8 deletions(-)