[PATCH 0/2] iwlwifi: Fix a crash at loading

Hi,

this is the fix for the recently introduced regression of iwlwifi
driver (since 5.10), a crash at the module loading [1][2].
The first patch actually fixes the bug, and this should go to 5.11-rc.
The second patch is an enhancement to make pointers const for safety.


Takashi

[1] https://bugzilla.suse.com/show_bug.cgi?id=1180344
[2] https://bugzilla.kernel.org/show_bug.cgi?id=210733

===

Takashi Iwai (2):
  iwlwifi: dbg: Don't touch the tlv data
  iwlwifi: dbg: Mark ucode tlv data as const

 .../net/wireless/intel/iwlwifi/iwl-dbg-tlv.c  | 57 +++++++++----------
 .../net/wireless/intel/iwlwifi/iwl-dbg-tlv.h  |  2 +-
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c  |  2 +-
 3 files changed, 28 insertions(+), 33 deletions(-)

-- 
2.26.2

The commit ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device
memory") added a termination of name string just to be sure, and this
seems causing a regression, a GPF triggered at firmware loading.
Basically we shouldn't modify the firmware data that may be provided
as read-only.

This patch drops the code that caused the regression and keep the tlv
data as is.

Fixes: ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory")
BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1180344
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210733
Signed-off-by: Takashi Iwai <redacted>
---

As an alternative fix, the debug print could be with the printf length
specifier to limit the size to IWL_FW_INIT_MAX_NAME, as found in the
bugzilla entries above, too.  I chose a simpler (partial) revert here
instead.

 drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
index a654147d3cd6..a80a35a7740f 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c

@@ -180,13 +180,6 @@ static int iwl_dbg_tlv_alloc_region(struct iwl_trans *trans,
 	if (le32_to_cpu(tlv->length) < sizeof(*reg))
 		return -EINVAL;
 
-	/* For safe using a string from FW make sure we have a
-	 * null terminator
-	 */
-	reg->name[IWL_FW_INI_MAX_NAME - 1] = 0;
-
-	IWL_DEBUG_FW(trans, "WRT: parsing region: %s\n", reg->name);
-
 	if (id >= IWL_FW_INI_MAX_REGION_ID) {
 		IWL_ERR(trans, "WRT: Invalid region id %u\n", id);
 		return -EINVAL;

-- 
2.26.2

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 <redacted>
---
 .../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(-)

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;

-- 
2.26.2

Takashi Iwai [off-list ref] writes:

The commit ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device
memory") added a termination of name string just to be sure, and this
seems causing a regression, a GPF triggered at firmware loading.
Basically we shouldn't modify the firmware data that may be provided
as read-only.

This patch drops the code that caused the regression and keep the tlv
data as is.

Fixes: ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory")
BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1180344
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210733
Signed-off-by: Takashi Iwai <redacted>

I'm planning to queue this to v5.11. Should I add cc stable?

Luca, can I have your ack?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Takashi Iwai [off-list ref] 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 <redacted>

So this need to go to -next, right? Does this depend on patch 1 or can
this be applied independently?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

On Tue, 12 Jan 2021 16:48:56 +0100,
Kalle Valo wrote:

Takashi Iwai [off-list ref] writes:

quoted

The commit ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device
memory") added a termination of name string just to be sure, and this
seems causing a regression, a GPF triggered at firmware loading.
Basically we shouldn't modify the firmware data that may be provided
as read-only.

This patch drops the code that caused the regression and keep the tlv
data as is.

Fixes: ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory")
BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1180344
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210733
Signed-off-by: Takashi Iwai <redacted>

I'm planning to queue this to v5.11. Should I add cc stable?

Yes, it hits 5.10.y.

Luca, can I have your ack?

It'd be great if this fix goes in quickly.


BTW, I thought network people don't want to have Cc-to-stable in the
patch, so I didn't put it by myself.  Is this rule still valid?


thanks,

Takashi

On Tue, 12 Jan 2021 16:50:54 +0100,
Kalle Valo wrote:

Takashi Iwai [off-list ref] writes:

quoted

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

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:02 +0100, Takashi Iwai wrote:

On Tue, 12 Jan 2021 16:48:56 +0100,
Kalle Valo wrote:

quoted

Takashi Iwai [off-list ref] writes:

quoted

The commit ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device
memory") added a termination of name string just to be sure, and this
seems causing a regression, a GPF triggered at firmware loading.
Basically we shouldn't modify the firmware data that may be provided
as read-only.

This patch drops the code that caused the regression and keep the tlv
data as is.

Fixes: ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory")
BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1180344
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210733
Signed-off-by: Takashi Iwai <redacted>

I'm planning to queue this to v5.11. Should I add cc stable?

Yes, it hits 5.10.y.

quoted

Luca, can I have your ack?

It'd be great if this fix goes in quickly.

Thanks for the fix!

Acked-by: Luca Coelho <redacted>

BTW, I thought network people don't want to have Cc-to-stable in the
patch, so I didn't put it by myself.  Is this rule still valid?

In the wireless side of network, we've always used Cc stable when
needed, but the Fixes tag itself will almost always trigger the stable
people to take it anyway.

--
Cheers,
Luca.

On Tue, 2021-01-12 at 17:05 +0100, Takashi Iwai wrote:

On Tue, 12 Jan 2021 16:50:54 +0100,
Kalle Valo wrote:

quoted

Takashi Iwai [off-list ref] writes:

quoted

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

So this need to go to -next, right?

Yes, this isn't urgently needed for 5.11.

Acked-by: Luca Coelho <redacted>

quoted

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" [off-list ref] writes:

On Tue, 2021-01-12 at 17:05 +0100, Takashi Iwai wrote:

quoted

On Tue, 12 Jan 2021 16:50:54 +0100,
Kalle Valo wrote:

quoted

Takashi Iwai [off-list ref] writes:

quoted

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

So this need to go to -next, right?

Yes, this isn't urgently needed for 5.11.

Acked-by: Luca Coelho <redacted>

quoted

quoted

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

"Coelho, Luciano" [off-list ref] writes:

quoted

BTW, I thought network people don't want to have Cc-to-stable in the
patch, so I didn't put it by myself.  Is this rule still valid?

In the wireless side of network, we've always used Cc stable when
needed

Yeah, we handle stable patches differently from the main network tree.

but the Fixes tag itself will almost always trigger the stable
people to take it anyway.

BTW, this is now clarified in the documentation:

https://lkml.kernel.org/r/20210113163315.1331064-1-lee.jones@linaro.org

So cc stable should be added even if there's already a Fixes tag.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Takashi Iwai [off-list ref] wrote:

The commit ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device
memory") added a termination of name string just to be sure, and this
seems causing a regression, a GPF triggered at firmware loading.
Basically we shouldn't modify the firmware data that may be provided
as read-only.

This patch drops the code that caused the regression and keep the tlv
data as is.

Fixes: ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory")
BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1180344
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210733
Cc: stable@vger.kernel.org
Signed-off-by: Takashi Iwai <redacted>
Acked-by: Luca Coelho <redacted>

Patch applied to wireless-drivers.git, thanks.

a6616bc9a0af iwlwifi: dbg: Don't touch the tlv data

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210112132449.22243-2-tiwai@suse.de/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Takashi Iwai [off-list ref] 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 <redacted>
Acked-by: Luca Coelho <redacted>

Patch applied to iwlwifi-next.git, thanks.

71b6254a6c98 iwlwifi: dbg: Mark ucode tlv data as const