| Message ID | 20210112132449.22243-3-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 71b6254a6c98eaf28f56749923c73b990dd905c1 |
| Delegated to: | Luca Coelho |
| Headers | show |
| Series | iwlwifi: Fix a crash at loading | expand |
Takashi Iwai <tiwai@suse.de> writes: > The ucode TLV data may be read-only and should be treated as const > pointers, but currently a few code forcibly cast to the writable > pointer unnecessarily. This gave developers a wrong impression as if > it can be modified, resulting in crashing regressions already a couple > of times. > > This patch adds the const prefix to those cast pointers, so that such > attempt can be caught more easily in future. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> So this need to go to -next, right? Does this depend on patch 1 or can this be applied independently?
On Tue, 12 Jan 2021 16:50:54 +0100, Kalle Valo wrote: > > Takashi Iwai <tiwai@suse.de> writes: > > > The ucode TLV data may be read-only and should be treated as const > > pointers, but currently a few code forcibly cast to the writable > > pointer unnecessarily. This gave developers a wrong impression as if > > it can be modified, resulting in crashing regressions already a couple > > of times. > > > > This patch adds the const prefix to those cast pointers, so that such > > attempt can be caught more easily in future. > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > So this need to go to -next, right? Yes, this isn't urgently needed for 5.11. > Does this depend on patch 1 or can > this be applied independently? It depends on the first patch, otherwise you'll get the warning in the code changing the const data (it must warn -- that's the purpose of this change :) So, if applying to a separate branch is difficult, applying together for 5.11 would be an option. thanks, Takashi
On Tue, 2021-01-12 at 17:05 +0100, Takashi Iwai wrote: > On Tue, 12 Jan 2021 16:50:54 +0100, > Kalle Valo wrote: > > > > Takashi Iwai <tiwai@suse.de> writes: > > > > > The ucode TLV data may be read-only and should be treated as const > > > pointers, but currently a few code forcibly cast to the writable > > > pointer unnecessarily. This gave developers a wrong impression as if > > > it can be modified, resulting in crashing regressions already a couple > > > of times. > > > > > > This patch adds the const prefix to those cast pointers, so that such > > > attempt can be caught more easily in future. > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > So this need to go to -next, right? > > Yes, this isn't urgently needed for 5.11. Acked-by: Luca Coelho <luciano.coelho@intel.com> > > Does this depend on patch 1 or can > > this be applied independently? > > It depends on the first patch, otherwise you'll get the warning in the > code changing the const data (it must warn -- that's the purpose of > this change :) > > So, if applying to a separate branch is difficult, applying together > for 5.11 would be an option. It doesn't matter to me how you apply it. Applying together is obviously going to be easier, but applying separately wouldn't be that hard either. You'd just have to track when 1/2 went into net-next before applying this one. Kalle's call. -- Cheers, Luca.
"Coelho, Luciano" <luciano.coelho@intel.com> writes: > On Tue, 2021-01-12 at 17:05 +0100, Takashi Iwai wrote: >> On Tue, 12 Jan 2021 16:50:54 +0100, >> Kalle Valo wrote: >> > >> > Takashi Iwai <tiwai@suse.de> writes: >> > >> > > The ucode TLV data may be read-only and should be treated as const >> > > pointers, but currently a few code forcibly cast to the writable >> > > pointer unnecessarily. This gave developers a wrong impression as if >> > > it can be modified, resulting in crashing regressions already a couple >> > > of times. >> > > >> > > This patch adds the const prefix to those cast pointers, so that such >> > > attempt can be caught more easily in future. >> > > >> > > Signed-off-by: Takashi Iwai <tiwai@suse.de> >> > >> > So this need to go to -next, right? >> >> Yes, this isn't urgently needed for 5.11. > > Acked-by: Luca Coelho <luciano.coelho@intel.com> > > >> > Does this depend on patch 1 or can >> > this be applied independently? >> >> It depends on the first patch, otherwise you'll get the warning in the >> code changing the const data (it must warn -- that's the purpose of >> this change :) >> >> So, if applying to a separate branch is difficult, applying together >> for 5.11 would be an option. > > It doesn't matter to me how you apply it. Applying together is > obviously going to be easier, but applying separately wouldn't be that > hard either. You'd just have to track when 1/2 went into net-next > before applying this one. Kalle's call. Ok, I'll apply this to wireless-drivers-next after wireless-drivers is merged to -next. It might take a while.
Takashi Iwai <tiwai@suse.de> wrote: > The ucode TLV data may be read-only and should be treated as const > pointers, but currently a few code forcibly cast to the writable > pointer unnecessarily. This gave developers a wrong impression as if > it can be modified, resulting in crashing regressions already a couple > of times. > > This patch adds the const prefix to those cast pointers, so that such > attempt can be caught more easily in future. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > Acked-by: Luca Coelho <luciano.coelho@intel.com> Patch applied to iwlwifi-next.git, thanks. 71b6254a6c98 iwlwifi: dbg: Mark ucode tlv data as const
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c index a80a35a7740f..12c49fe8608a 100644 --- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c +++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c @@ -61,7 +61,8 @@ dbg_ver_table[IWL_DBG_TLV_TYPE_NUM] = { [IWL_DBG_TLV_TYPE_TRIGGER] = {.min_ver = 1, .max_ver = 1,}, }; -static int iwl_dbg_tlv_add(struct iwl_ucode_tlv *tlv, struct list_head *list) +static int iwl_dbg_tlv_add(const struct iwl_ucode_tlv *tlv, + struct list_head *list) { u32 len = le32_to_cpu(tlv->length); struct iwl_dbg_tlv_node *node; @@ -76,9 +77,9 @@ static int iwl_dbg_tlv_add(struct iwl_ucode_tlv *tlv, struct list_head *list) return 0; } -static bool iwl_dbg_tlv_ver_support(struct iwl_ucode_tlv *tlv) +static bool iwl_dbg_tlv_ver_support(const struct iwl_ucode_tlv *tlv) { - struct iwl_fw_ini_header *hdr = (void *)&tlv->data[0]; + const struct iwl_fw_ini_header *hdr = (const void *)&tlv->data[0]; u32 type = le32_to_cpu(tlv->type); u32 tlv_idx = type - IWL_UCODE_TLV_DEBUG_BASE; u32 ver = le32_to_cpu(hdr->version); @@ -91,9 +92,9 @@ static bool iwl_dbg_tlv_ver_support(struct iwl_ucode_tlv *tlv) } static int iwl_dbg_tlv_alloc_debug_info(struct iwl_trans *trans, - struct iwl_ucode_tlv *tlv) + const struct iwl_ucode_tlv *tlv) { - struct iwl_fw_ini_debug_info_tlv *debug_info = (void *)tlv->data; + const struct iwl_fw_ini_debug_info_tlv *debug_info = (const void *)tlv->data; if (le32_to_cpu(tlv->length) != sizeof(*debug_info)) return -EINVAL; @@ -105,9 +106,9 @@ static int iwl_dbg_tlv_alloc_debug_info(struct iwl_trans *trans, } static int iwl_dbg_tlv_alloc_buf_alloc(struct iwl_trans *trans, - struct iwl_ucode_tlv *tlv) + const struct iwl_ucode_tlv *tlv) { - struct iwl_fw_ini_allocation_tlv *alloc = (void *)tlv->data; + const struct iwl_fw_ini_allocation_tlv *alloc = (const void *)tlv->data; u32 buf_location; u32 alloc_id; @@ -145,9 +146,9 @@ static int iwl_dbg_tlv_alloc_buf_alloc(struct iwl_trans *trans, } static int iwl_dbg_tlv_alloc_hcmd(struct iwl_trans *trans, - struct iwl_ucode_tlv *tlv) + const struct iwl_ucode_tlv *tlv) { - struct iwl_fw_ini_hcmd_tlv *hcmd = (void *)tlv->data; + const struct iwl_fw_ini_hcmd_tlv *hcmd = (const void *)tlv->data; u32 tp = le32_to_cpu(hcmd->time_point); if (le32_to_cpu(tlv->length) <= sizeof(*hcmd)) @@ -169,9 +170,9 @@ static int iwl_dbg_tlv_alloc_hcmd(struct iwl_trans *trans, } static int iwl_dbg_tlv_alloc_region(struct iwl_trans *trans, - struct iwl_ucode_tlv *tlv) + const struct iwl_ucode_tlv *tlv) { - struct iwl_fw_ini_region_tlv *reg = (void *)tlv->data; + const struct iwl_fw_ini_region_tlv *reg = (const void *)tlv->data; struct iwl_ucode_tlv **active_reg; u32 id = le32_to_cpu(reg->id); u32 type = le32_to_cpu(reg->type); @@ -214,9 +215,10 @@ static int iwl_dbg_tlv_alloc_region(struct iwl_trans *trans, } static int iwl_dbg_tlv_alloc_trigger(struct iwl_trans *trans, - struct iwl_ucode_tlv *tlv) + const struct iwl_ucode_tlv *tlv) { - struct iwl_fw_ini_trigger_tlv *trig = (void *)tlv->data; + const struct iwl_fw_ini_trigger_tlv *trig = (const void *)tlv->data; + struct iwl_fw_ini_trigger_tlv *dup_trig; u32 tp = le32_to_cpu(trig->time_point); struct iwl_ucode_tlv *dup = NULL; int ret; @@ -237,8 +239,8 @@ static int iwl_dbg_tlv_alloc_trigger(struct iwl_trans *trans, GFP_KERNEL); if (!dup) return -ENOMEM; - trig = (void *)dup->data; - trig->occurrences = cpu_to_le32(-1); + dup_trig = (void *)dup->data; + dup_trig->occurrences = cpu_to_le32(-1); tlv = dup; } @@ -249,7 +251,7 @@ static int iwl_dbg_tlv_alloc_trigger(struct iwl_trans *trans, } static int (*dbg_tlv_alloc[])(struct iwl_trans *trans, - struct iwl_ucode_tlv *tlv) = { + const struct iwl_ucode_tlv *tlv) = { [IWL_DBG_TLV_TYPE_DEBUG_INFO] = iwl_dbg_tlv_alloc_debug_info, [IWL_DBG_TLV_TYPE_BUF_ALLOC] = iwl_dbg_tlv_alloc_buf_alloc, [IWL_DBG_TLV_TYPE_HCMD] = iwl_dbg_tlv_alloc_hcmd, @@ -257,10 +259,10 @@ static int (*dbg_tlv_alloc[])(struct iwl_trans *trans, [IWL_DBG_TLV_TYPE_TRIGGER] = iwl_dbg_tlv_alloc_trigger, }; -void iwl_dbg_tlv_alloc(struct iwl_trans *trans, struct iwl_ucode_tlv *tlv, +void iwl_dbg_tlv_alloc(struct iwl_trans *trans, const struct iwl_ucode_tlv *tlv, bool ext) { - struct iwl_fw_ini_header *hdr = (void *)&tlv->data[0]; + const struct iwl_fw_ini_header *hdr = (const void *)&tlv->data[0]; u32 type = le32_to_cpu(tlv->type); u32 tlv_idx = type - IWL_UCODE_TLV_DEBUG_BASE; u32 domain = le32_to_cpu(hdr->domain); @@ -396,7 +398,7 @@ void iwl_dbg_tlv_free(struct iwl_trans *trans) static int iwl_dbg_tlv_parse_bin(struct iwl_trans *trans, const u8 *data, size_t len) { - struct iwl_ucode_tlv *tlv; + const struct iwl_ucode_tlv *tlv; u32 tlv_len; while (len >= sizeof(*tlv)) { @@ -737,12 +739,12 @@ static void iwl_dbg_tlv_set_periodic_trigs(struct iwl_fw_runtime *fwrt) } } -static bool is_trig_data_contained(struct iwl_ucode_tlv *new, - struct iwl_ucode_tlv *old) +static bool is_trig_data_contained(const struct iwl_ucode_tlv *new, + const struct iwl_ucode_tlv *old) { - struct iwl_fw_ini_trigger_tlv *new_trig = (void *)new->data; - struct iwl_fw_ini_trigger_tlv *old_trig = (void *)old->data; - __le32 *new_data = new_trig->data, *old_data = old_trig->data; + const struct iwl_fw_ini_trigger_tlv *new_trig = (const void *)new->data; + const struct iwl_fw_ini_trigger_tlv *old_trig = (const void *)old->data; + const __le32 *new_data = new_trig->data, *old_data = old_trig->data; u32 new_dwords_num = iwl_tlv_array_len(new, new_trig, data); u32 old_dwords_num = iwl_tlv_array_len(old, old_trig, data); int i, j; diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.h b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.h index 246823878281..e9f19ecbc4ee 100644 --- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.h +++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.h @@ -43,7 +43,7 @@ struct iwl_fw_runtime; void iwl_dbg_tlv_load_bin(struct device *dev, struct iwl_trans *trans); void iwl_dbg_tlv_free(struct iwl_trans *trans); -void iwl_dbg_tlv_alloc(struct iwl_trans *trans, struct iwl_ucode_tlv *tlv, +void iwl_dbg_tlv_alloc(struct iwl_trans *trans, const struct iwl_ucode_tlv *tlv, bool ext); void iwl_dbg_tlv_init(struct iwl_trans *trans); void iwl_dbg_tlv_time_point(struct iwl_fw_runtime *fwrt, diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c index d44bc61c34f5..5dcc490729b4 100644 --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c @@ -558,7 +558,7 @@ static int iwl_parse_tlv_firmware(struct iwl_drv *drv, bool *usniffer_images) { struct iwl_tlv_ucode_header *ucode = (void *)ucode_raw->data; - struct iwl_ucode_tlv *tlv; + const struct iwl_ucode_tlv *tlv; size_t len = ucode_raw->size; const u8 *data; u32 tlv_len;
The ucode TLV data may be read-only and should be treated as const pointers, but currently a few code forcibly cast to the writable pointer unnecessarily. This gave developers a wrong impression as if it can be modified, resulting in crashing regressions already a couple of times. This patch adds the const prefix to those cast pointers, so that such attempt can be caught more easily in future. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- .../net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 50 ++++++++++--------- .../net/wireless/intel/iwlwifi/iwl-dbg-tlv.h | 2 +- drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 2 +- 3 files changed, 28 insertions(+), 26 deletions(-)