| Message ID | 20220901204558.2256458-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0d24201f47c4270043314705fcc13e1d7c675527 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | iwlwifi: calib: Refactor iwl_calib_result usage for clarity | expand |
Kees Cook <keescook@chromium.org> writes: > In preparation for FORTIFY_SOURCE performing run-time destination buffer > bounds checking for memcpy(), refactor the use of struct iwl_calib_result: > > - Have struct iwl_calib_result contain struct iwl_calib_cmd since > functions expect to operate on the "data" flex array in "cmd", which > follows the "hdr" member. > - Switch argument passing around to use struct iwl_calib_cmd instead of > struct iwl_calib_hdr to prepare functions to see the "data" member. > - Change iwl_calib_set()'s "len" argument to a size_t since it is always > unsigned and is normally receiving the output of sizeof(). > - Add an explicit length sanity check in iwl_calib_set(). > - Adjust the memcpy() to avoid copying across the now visible composite > flex array structure. > > This avoids the future run-time warning: > > memcpy: detected field-spanning write (size 8) of single field "&res->hdr" (size 4) > > Cc: Luca Coelho <luciano.coelho@intel.com> > Cc: Kalle Valo <kvalo@codeaurora.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Lee Jones <lee.jones@linaro.org> > Cc: Johannes Berg <johannes.berg@intel.com> > Cc: linux-wireless@vger.kernel.org > Cc: netdev@vger.kernel.org > Reported-by: Andy Lavr <andy.lavr@gmail.com> > Signed-off-by: Kees Cook <keescook@chromium.org> Gregory, as this fixes a future warning can I take this directly to wireless-next?
Kees Cook <keescook@chromium.org> wrote: > In preparation for FORTIFY_SOURCE performing run-time destination buffer > bounds checking for memcpy(), refactor the use of struct iwl_calib_result: > > - Have struct iwl_calib_result contain struct iwl_calib_cmd since > functions expect to operate on the "data" flex array in "cmd", which > follows the "hdr" member. > - Switch argument passing around to use struct iwl_calib_cmd instead of > struct iwl_calib_hdr to prepare functions to see the "data" member. > - Change iwl_calib_set()'s "len" argument to a size_t since it is always > unsigned and is normally receiving the output of sizeof(). > - Add an explicit length sanity check in iwl_calib_set(). > - Adjust the memcpy() to avoid copying across the now visible composite > flex array structure. > > This avoids the future run-time warning: > > memcpy: detected field-spanning write (size 8) of single field "&res->hdr" (size 4) > > Cc: Luca Coelho <luciano.coelho@intel.com> > Cc: Kalle Valo <kvalo@codeaurora.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Lee Jones <lee.jones@linaro.org> > Cc: Johannes Berg <johannes.berg@intel.com> > Cc: linux-wireless@vger.kernel.org > Cc: netdev@vger.kernel.org > Reported-by: Andy Lavr <andy.lavr@gmail.com> > Signed-off-by: Kees Cook <keescook@chromium.org> Patch applied to wireless-next.git, thanks. 0d24201f47c4 wifi: iwlwifi: calib: Refactor iwl_calib_result usage for clarity
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/agn.h b/drivers/net/wireless/intel/iwlwifi/dvm/agn.h index 411a6f6638b4..fefaa414272b 100644 --- a/drivers/net/wireless/intel/iwlwifi/dvm/agn.h +++ b/drivers/net/wireless/intel/iwlwifi/dvm/agn.h @@ -112,7 +112,7 @@ int iwl_load_ucode_wait_alive(struct iwl_priv *priv, enum iwl_ucode_type ucode_type); int iwl_send_calib_results(struct iwl_priv *priv); int iwl_calib_set(struct iwl_priv *priv, - const struct iwl_calib_hdr *cmd, int len); + const struct iwl_calib_cmd *cmd, size_t len); void iwl_calib_free_results(struct iwl_priv *priv); int iwl_dump_nic_event_log(struct iwl_priv *priv, bool full_log, char **buf); diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/calib.c b/drivers/net/wireless/intel/iwlwifi/dvm/calib.c index a11884fa254b..f488620d2844 100644 --- a/drivers/net/wireless/intel/iwlwifi/dvm/calib.c +++ b/drivers/net/wireless/intel/iwlwifi/dvm/calib.c @@ -19,8 +19,7 @@ struct iwl_calib_result { struct list_head list; size_t cmd_len; - struct iwl_calib_hdr hdr; - /* data follows */ + struct iwl_calib_cmd cmd; }; struct statistics_general_data { @@ -43,12 +42,12 @@ int iwl_send_calib_results(struct iwl_priv *priv) int ret; hcmd.len[0] = res->cmd_len; - hcmd.data[0] = &res->hdr; + hcmd.data[0] = &res->cmd; hcmd.dataflags[0] = IWL_HCMD_DFL_NOCOPY; ret = iwl_dvm_send_cmd(priv, &hcmd); if (ret) { IWL_ERR(priv, "Error %d on calib cmd %d\n", - ret, res->hdr.op_code); + ret, res->cmd.hdr.op_code); return ret; } } @@ -57,19 +56,22 @@ int iwl_send_calib_results(struct iwl_priv *priv) } int iwl_calib_set(struct iwl_priv *priv, - const struct iwl_calib_hdr *cmd, int len) + const struct iwl_calib_cmd *cmd, size_t len) { struct iwl_calib_result *res, *tmp; - res = kmalloc(sizeof(*res) + len - sizeof(struct iwl_calib_hdr), - GFP_ATOMIC); + if (check_sub_overflow(len, sizeof(*cmd), &len)) + return -ENOMEM; + + res = kmalloc(struct_size(res, cmd.data, len), GFP_ATOMIC); if (!res) return -ENOMEM; - memcpy(&res->hdr, cmd, len); - res->cmd_len = len; + res->cmd = *cmd; + memcpy(res->cmd.data, cmd->data, len); + res->cmd_len = struct_size(cmd, data, len); list_for_each_entry(tmp, &priv->calib_results, list) { - if (tmp->hdr.op_code == res->hdr.op_code) { + if (tmp->cmd.hdr.op_code == res->cmd.hdr.op_code) { list_replace(&tmp->list, &res->list); kfree(tmp); return 0; diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/ucode.c b/drivers/net/wireless/intel/iwlwifi/dvm/ucode.c index 4b27a53d0bb4..bb13ca5d666c 100644 --- a/drivers/net/wireless/intel/iwlwifi/dvm/ucode.c +++ b/drivers/net/wireless/intel/iwlwifi/dvm/ucode.c @@ -356,18 +356,18 @@ static bool iwlagn_wait_calib(struct iwl_notif_wait_data *notif_wait, struct iwl_rx_packet *pkt, void *data) { struct iwl_priv *priv = data; - struct iwl_calib_hdr *hdr; + struct iwl_calib_cmd *cmd; if (pkt->hdr.cmd != CALIBRATION_RES_NOTIFICATION) { WARN_ON(pkt->hdr.cmd != CALIBRATION_COMPLETE_NOTIFICATION); return true; } - hdr = (struct iwl_calib_hdr *)pkt->data; + cmd = (struct iwl_calib_cmd *)pkt->data; - if (iwl_calib_set(priv, hdr, iwl_rx_packet_payload_len(pkt))) + if (iwl_calib_set(priv, cmd, iwl_rx_packet_payload_len(pkt))) IWL_ERR(priv, "Failed to record calibration data %d\n", - hdr->op_code); + cmd->hdr.op_code); return false; }
In preparation for FORTIFY_SOURCE performing run-time destination buffer bounds checking for memcpy(), refactor the use of struct iwl_calib_result: - Have struct iwl_calib_result contain struct iwl_calib_cmd since functions expect to operate on the "data" flex array in "cmd", which follows the "hdr" member. - Switch argument passing around to use struct iwl_calib_cmd instead of struct iwl_calib_hdr to prepare functions to see the "data" member. - Change iwl_calib_set()'s "len" argument to a size_t since it is always unsigned and is normally receiving the output of sizeof(). - Add an explicit length sanity check in iwl_calib_set(). - Adjust the memcpy() to avoid copying across the now visible composite flex array structure. This avoids the future run-time warning: memcpy: detected field-spanning write (size 8) of single field "&res->hdr" (size 4) Cc: Luca Coelho <luciano.coelho@intel.com> Cc: Kalle Valo <kvalo@codeaurora.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Lee Jones <lee.jones@linaro.org> Cc: Johannes Berg <johannes.berg@intel.com> Cc: linux-wireless@vger.kernel.org Cc: netdev@vger.kernel.org Reported-by: Andy Lavr <andy.lavr@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/net/wireless/intel/iwlwifi/dvm/agn.h | 2 +- .../net/wireless/intel/iwlwifi/dvm/calib.c | 22 ++++++++++--------- .../net/wireless/intel/iwlwifi/dvm/ucode.c | 8 +++---- 3 files changed, 17 insertions(+), 15 deletions(-)