[PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw features query.

[PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw features query.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Stop referencing 'features' memory after release_firmware is called.

Fixes this crash:

RIP: 0010:mt7921_check_offload_capability+0x17d
mt7921_pci_probe+0xca/0x4b0
...

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/mediatek/mt76/mt7921/init.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
index 38d6563cb12f..d2bb8d02ce0a 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
@@ -165,12 +165,12 @@ mt7921_mac_init_band(struct mt7921_dev *dev, u8 band)
 
 u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
 {
-	struct mt7921_fw_features *features = NULL;
 	const struct mt76_connac2_fw_trailer *hdr;
 	struct mt7921_realease_info *rel_info;
 	const struct firmware *fw;
 	int ret, i, offset = 0;
 	const u8 *data, *end;
+	u8 offload_caps = 0;
 
 	ret = request_firmware(&fw, fw_wm, dev);
 	if (ret)
@@ -197,12 +197,19 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
 	data += sizeof(*rel_info);
 	end = data + le16_to_cpu(rel_info->len);
 
+	/* TODO:  This needs better sanity checking I think.
+	 * Likely a corrupted firmware with bad rel_info->len, for instance,
+	 * would blow this up.
+	 */
 	while (data < end) {
 		rel_info = (struct mt7921_realease_info *)data;
 		data += sizeof(*rel_info);
 
 		if (rel_info->tag == MT7921_FW_TAG_FEATURE) {
+			struct mt7921_fw_features *features;
+
 			features = (struct mt7921_fw_features *)data;
+			offload_caps = features->data;
 			break;
 		}
 
@@ -211,7 +218,7 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
 
 	release_firmware(fw);
 
-	return features ? features->data : 0;
+	return offload_caps;
 }
 EXPORT_SYMBOL_GPL(mt7921_check_offload_capability);
 
-- 
2.39.1

Re: [PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw features query.

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]

> From: Ben Greear <greearb@candelatech.com>
> 
> Stop referencing 'features' memory after release_firmware is called.
> 
> Fixes this crash:
> 
> RIP: 0010:mt7921_check_offload_capability+0x17d
> mt7921_pci_probe+0xca/0x4b0
> ...
> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> ---
>  drivers/net/wireless/mediatek/mt76/mt7921/init.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> index 38d6563cb12f..d2bb8d02ce0a 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> @@ -165,12 +165,12 @@ mt7921_mac_init_band(struct mt7921_dev *dev, u8 band)
>  
>  u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>  {
> -	struct mt7921_fw_features *features = NULL;
>  	const struct mt76_connac2_fw_trailer *hdr;
>  	struct mt7921_realease_info *rel_info;
>  	const struct firmware *fw;
>  	int ret, i, offset = 0;
>  	const u8 *data, *end;
> +	u8 offload_caps = 0;
>  
>  	ret = request_firmware(&fw, fw_wm, dev);
>  	if (ret)
> @@ -197,12 +197,19 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>  	data += sizeof(*rel_info);
>  	end = data + le16_to_cpu(rel_info->len);
>  
> +	/* TODO:  This needs better sanity checking I think.
> +	 * Likely a corrupted firmware with bad rel_info->len, for instance,
> +	 * would blow this up.
> +	 */
>  	while (data < end) {
>  		rel_info = (struct mt7921_realease_info *)data;
>  		data += sizeof(*rel_info);
>  
>  		if (rel_info->tag == MT7921_FW_TAG_FEATURE) {
> +			struct mt7921_fw_features *features;
> +
>  			features = (struct mt7921_fw_features *)data;
> +			offload_caps = features->data;
>  			break;
>  		}
>  
> @@ -211,7 +218,7 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>  
>  	release_firmware(fw);
>  
> -	return features ? features->data : 0;
> +	return offload_caps;
>  }
>  EXPORT_SYMBOL_GPL(mt7921_check_offload_capability);
>  
> -- 
> 2.39.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw features query.

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

> From: Ben Greear <greearb@candelatech.com>
> 
> Stop referencing 'features' memory after release_firmware is called.
> 
> Fixes this crash:
> 
> RIP: 0010:mt7921_check_offload_capability+0x17d
> mt7921_pci_probe+0xca/0x4b0
> ...

I would say even this patch can go trough wireless tree.

@Felix: agree?

Regards,
Lorenzo

> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  drivers/net/wireless/mediatek/mt76/mt7921/init.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> index 38d6563cb12f..d2bb8d02ce0a 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> @@ -165,12 +165,12 @@ mt7921_mac_init_band(struct mt7921_dev *dev, u8 band)
>  
>  u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>  {
> -	struct mt7921_fw_features *features = NULL;
>  	const struct mt76_connac2_fw_trailer *hdr;
>  	struct mt7921_realease_info *rel_info;
>  	const struct firmware *fw;
>  	int ret, i, offset = 0;
>  	const u8 *data, *end;
> +	u8 offload_caps = 0;
>  
>  	ret = request_firmware(&fw, fw_wm, dev);
>  	if (ret)
> @@ -197,12 +197,19 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>  	data += sizeof(*rel_info);
>  	end = data + le16_to_cpu(rel_info->len);
>  
> +	/* TODO:  This needs better sanity checking I think.
> +	 * Likely a corrupted firmware with bad rel_info->len, for instance,
> +	 * would blow this up.
> +	 */
>  	while (data < end) {
>  		rel_info = (struct mt7921_realease_info *)data;
>  		data += sizeof(*rel_info);
>  
>  		if (rel_info->tag == MT7921_FW_TAG_FEATURE) {
> +			struct mt7921_fw_features *features;
> +
>  			features = (struct mt7921_fw_features *)data;
> +			offload_caps = features->data;
>  			break;
>  		}
>  
> @@ -211,7 +218,7 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>  
>  	release_firmware(fw);
>  
> -	return features ? features->data : 0;
> +	return offload_caps;
>  }
>  EXPORT_SYMBOL_GPL(mt7921_check_offload_capability);
>  
> -- 
> 2.39.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw features query.

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

> From: Ben Greear <greearb@candelatech.com>
> 
> Stop referencing 'features' memory after release_firmware is called.
> 
> Fixes this crash:
> 
> RIP: 0010:mt7921_check_offload_capability+0x17d
> mt7921_pci_probe+0xca/0x4b0
> ...
> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  drivers/net/wireless/mediatek/mt76/mt7921/init.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> index 38d6563cb12f..d2bb8d02ce0a 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> @@ -165,12 +165,12 @@ mt7921_mac_init_band(struct mt7921_dev *dev, u8 band)
>  
>  u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>  {
> -	struct mt7921_fw_features *features = NULL;
>  	const struct mt76_connac2_fw_trailer *hdr;
>  	struct mt7921_realease_info *rel_info;
>  	const struct firmware *fw;
>  	int ret, i, offset = 0;
>  	const u8 *data, *end;
> +	u8 offload_caps = 0;
>  
>  	ret = request_firmware(&fw, fw_wm, dev);
>  	if (ret)
> @@ -197,12 +197,19 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>  	data += sizeof(*rel_info);
>  	end = data + le16_to_cpu(rel_info->len);
>  
> +	/* TODO:  This needs better sanity checking I think.
> +	 * Likely a corrupted firmware with bad rel_info->len, for instance,
> +	 * would blow this up.
> +	 */

can you please repost dropping this comment?

Regards,
Lorenzo

>  	while (data < end) {
>  		rel_info = (struct mt7921_realease_info *)data;
>  		data += sizeof(*rel_info);
>  
>  		if (rel_info->tag == MT7921_FW_TAG_FEATURE) {
> +			struct mt7921_fw_features *features;
> +
>  			features = (struct mt7921_fw_features *)data;
> +			offload_caps = features->data;
>  			break;
>  		}
>  
> @@ -211,7 +218,7 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>  
>  	release_firmware(fw);
>  
> -	return features ? features->data : 0;
> +	return offload_caps;
>  }
>  EXPORT_SYMBOL_GPL(mt7921_check_offload_capability);
>  
> -- 
> 2.39.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw features query.

[Ben Greear]

On 3/21/23 02:58, Lorenzo Bianconi wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Stop referencing 'features' memory after release_firmware is called.
>>
>> Fixes this crash:
>>
>> RIP: 0010:mt7921_check_offload_capability+0x17d
>> mt7921_pci_probe+0xca/0x4b0
>> ...
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>   drivers/net/wireless/mediatek/mt76/mt7921/init.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
>> index 38d6563cb12f..d2bb8d02ce0a 100644
>> --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
>> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
>> @@ -165,12 +165,12 @@ mt7921_mac_init_band(struct mt7921_dev *dev, u8 band)
>>   
>>   u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>>   {
>> -	struct mt7921_fw_features *features = NULL;
>>   	const struct mt76_connac2_fw_trailer *hdr;
>>   	struct mt7921_realease_info *rel_info;
>>   	const struct firmware *fw;
>>   	int ret, i, offset = 0;
>>   	const u8 *data, *end;
>> +	u8 offload_caps = 0;
>>   
>>   	ret = request_firmware(&fw, fw_wm, dev);
>>   	if (ret)
>> @@ -197,12 +197,19 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>>   	data += sizeof(*rel_info);
>>   	end = data + le16_to_cpu(rel_info->len);
>>   
>> +	/* TODO:  This needs better sanity checking I think.
>> +	 * Likely a corrupted firmware with bad rel_info->len, for instance,
>> +	 * would blow this up.
>> +	 */
> 
> can you please repost dropping this comment?

Why?  Looks to me like this portion of mtk driver logic assumes firmware is
never corrupted on accident or on purpose.  It should be fixed at some point.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw features query.

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

> On 3/21/23 02:58, Lorenzo Bianconi wrote:
> > > From: Ben Greear <greearb@candelatech.com>
> > > 
> > > Stop referencing 'features' memory after release_firmware is called.
> > > 
> > > Fixes this crash:
> > > 
> > > RIP: 0010:mt7921_check_offload_capability+0x17d
> > > mt7921_pci_probe+0xca/0x4b0
> > > ...
> > > 
> > > Signed-off-by: Ben Greear <greearb@candelatech.com>
> > > ---
> > >   drivers/net/wireless/mediatek/mt76/mt7921/init.c | 11 +++++++++--
> > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> > > index 38d6563cb12f..d2bb8d02ce0a 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> > > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> > > @@ -165,12 +165,12 @@ mt7921_mac_init_band(struct mt7921_dev *dev, u8 band)
> > >   u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
> > >   {
> > > -	struct mt7921_fw_features *features = NULL;
> > >   	const struct mt76_connac2_fw_trailer *hdr;
> > >   	struct mt7921_realease_info *rel_info;
> > >   	const struct firmware *fw;
> > >   	int ret, i, offset = 0;
> > >   	const u8 *data, *end;
> > > +	u8 offload_caps = 0;
> > >   	ret = request_firmware(&fw, fw_wm, dev);
> > >   	if (ret)
> > > @@ -197,12 +197,19 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
> > >   	data += sizeof(*rel_info);
> > >   	end = data + le16_to_cpu(rel_info->len);
> > > +	/* TODO:  This needs better sanity checking I think.
> > > +	 * Likely a corrupted firmware with bad rel_info->len, for instance,
> > > +	 * would blow this up.
> > > +	 */
> > 
> > can you please repost dropping this comment?
> 
> Why?  Looks to me like this portion of mtk driver logic assumes firmware is
> never corrupted on accident or on purpose.  It should be fixed at some point.

even if this is a theoretical issue, this does not seem the right way to track
it and it is not related to this patch.

Regards,
Lorenzo

> 
> Thanks,
> Ben
> 
> -- 
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw features query.

[Ben Greear]

On 3/21/23 06:28, Lorenzo Bianconi wrote:
>> On 3/21/23 02:58, Lorenzo Bianconi wrote:
>>>> From: Ben Greear <greearb@candelatech.com>
>>>>
>>>> Stop referencing 'features' memory after release_firmware is called.
>>>>
>>>> Fixes this crash:
>>>>
>>>> RIP: 0010:mt7921_check_offload_capability+0x17d
>>>> mt7921_pci_probe+0xca/0x4b0
>>>> ...
>>>>
>>>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>>>> ---
>>>>    drivers/net/wireless/mediatek/mt76/mt7921/init.c | 11 +++++++++--
>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
>>>> index 38d6563cb12f..d2bb8d02ce0a 100644
>>>> --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
>>>> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
>>>> @@ -165,12 +165,12 @@ mt7921_mac_init_band(struct mt7921_dev *dev, u8 band)
>>>>    u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>>>>    {
>>>> -	struct mt7921_fw_features *features = NULL;
>>>>    	const struct mt76_connac2_fw_trailer *hdr;
>>>>    	struct mt7921_realease_info *rel_info;
>>>>    	const struct firmware *fw;
>>>>    	int ret, i, offset = 0;
>>>>    	const u8 *data, *end;
>>>> +	u8 offload_caps = 0;
>>>>    	ret = request_firmware(&fw, fw_wm, dev);
>>>>    	if (ret)
>>>> @@ -197,12 +197,19 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>>>>    	data += sizeof(*rel_info);
>>>>    	end = data + le16_to_cpu(rel_info->len);
>>>> +	/* TODO:  This needs better sanity checking I think.
>>>> +	 * Likely a corrupted firmware with bad rel_info->len, for instance,
>>>> +	 * would blow this up.
>>>> +	 */
>>>
>>> can you please repost dropping this comment?
>>
>> Why?  Looks to me like this portion of mtk driver logic assumes firmware is
>> never corrupted on accident or on purpose.  It should be fixed at some point.
> 
> even if this is a theoretical issue, this does not seem the right way to track
> it and it is not related to this patch.

There is no better way I know to track such problems, it is not like random OSS developers will
read kernel bugzilla for this sort of thing, and email bug report about it will be buried in days
and never seen again.

And it is related to crazy memory corruption bugs since a corrupted firmware could cause similar
memory corruption.  So next time someone is working on this code, maybe they will see the
comment and code it more defensively.

Anyway, if you insist, I can re-spin the patch against 6.2 and repost.  I also hear that it will not
apply to 6.3, but I'm not yet working on 6.3.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw features query.

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3319 bytes --]

> On 3/21/23 06:28, Lorenzo Bianconi wrote:
> > > On 3/21/23 02:58, Lorenzo Bianconi wrote:
> > > > > From: Ben Greear <greearb@candelatech.com>
> > > > > 
> > > > > Stop referencing 'features' memory after release_firmware is called.
> > > > > 
> > > > > Fixes this crash:
> > > > > 
> > > > > RIP: 0010:mt7921_check_offload_capability+0x17d
> > > > > mt7921_pci_probe+0xca/0x4b0
> > > > > ...
> > > > > 
> > > > > Signed-off-by: Ben Greear <greearb@candelatech.com>
> > > > > ---
> > > > >    drivers/net/wireless/mediatek/mt76/mt7921/init.c | 11 +++++++++--
> > > > >    1 file changed, 9 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> > > > > index 38d6563cb12f..d2bb8d02ce0a 100644
> > > > > --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> > > > > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> > > > > @@ -165,12 +165,12 @@ mt7921_mac_init_band(struct mt7921_dev *dev, u8 band)
> > > > >    u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
> > > > >    {
> > > > > -	struct mt7921_fw_features *features = NULL;
> > > > >    	const struct mt76_connac2_fw_trailer *hdr;
> > > > >    	struct mt7921_realease_info *rel_info;
> > > > >    	const struct firmware *fw;
> > > > >    	int ret, i, offset = 0;
> > > > >    	const u8 *data, *end;
> > > > > +	u8 offload_caps = 0;
> > > > >    	ret = request_firmware(&fw, fw_wm, dev);
> > > > >    	if (ret)
> > > > > @@ -197,12 +197,19 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
> > > > >    	data += sizeof(*rel_info);
> > > > >    	end = data + le16_to_cpu(rel_info->len);
> > > > > +	/* TODO:  This needs better sanity checking I think.
> > > > > +	 * Likely a corrupted firmware with bad rel_info->len, for instance,
> > > > > +	 * would blow this up.
> > > > > +	 */
> > > > 
> > > > can you please repost dropping this comment?
> > > 
> > > Why?  Looks to me like this portion of mtk driver logic assumes firmware is
> > > never corrupted on accident or on purpose.  It should be fixed at some point.
> > 
> > even if this is a theoretical issue, this does not seem the right way to track
> > it and it is not related to this patch.
> 
> There is no better way I know to track such problems, it is not like random OSS developers will
> read kernel bugzilla for this sort of thing, and email bug report about it will be buried in days
> and never seen again.
> 
> And it is related to crazy memory corruption bugs since a corrupted firmware could cause similar
> memory corruption.  So next time someone is working on this code, maybe they will see the
> comment and code it more defensively.

honestly I do not think this comment is so useful to understand a memory corruption issue,
it just makes the code uglier. If you feel this is a real issue, I would say to post a
patch to improve the code :).

Regards,
Lorenzo

> 
> Anyway, if you insist, I can re-spin the patch against 6.2 and repost.  I also hear that it will not
> apply to 6.3, but I'm not yet working on 6.3.
> 
> Thanks,
> Ben
> 
> -- 
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw features query.

[Ben Greear]

On 3/21/23 07:37, Lorenzo Bianconi wrote:
>> On 3/21/23 06:28, Lorenzo Bianconi wrote:
>>>> On 3/21/23 02:58, Lorenzo Bianconi wrote:
>>>>>> From: Ben Greear <greearb@candelatech.com>
>>>>>>
>>>>>> Stop referencing 'features' memory after release_firmware is called.
>>>>>>
>>>>>> Fixes this crash:
>>>>>>
>>>>>> RIP: 0010:mt7921_check_offload_capability+0x17d
>>>>>> mt7921_pci_probe+0xca/0x4b0
>>>>>> ...
>>>>>>
>>>>>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>>>>>> ---
>>>>>>     drivers/net/wireless/mediatek/mt76/mt7921/init.c | 11 +++++++++--
>>>>>>     1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
>>>>>> index 38d6563cb12f..d2bb8d02ce0a 100644
>>>>>> --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
>>>>>> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
>>>>>> @@ -165,12 +165,12 @@ mt7921_mac_init_band(struct mt7921_dev *dev, u8 band)
>>>>>>     u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>>>>>>     {
>>>>>> -	struct mt7921_fw_features *features = NULL;
>>>>>>     	const struct mt76_connac2_fw_trailer *hdr;
>>>>>>     	struct mt7921_realease_info *rel_info;
>>>>>>     	const struct firmware *fw;
>>>>>>     	int ret, i, offset = 0;
>>>>>>     	const u8 *data, *end;
>>>>>> +	u8 offload_caps = 0;
>>>>>>     	ret = request_firmware(&fw, fw_wm, dev);
>>>>>>     	if (ret)
>>>>>> @@ -197,12 +197,19 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm)
>>>>>>     	data += sizeof(*rel_info);
>>>>>>     	end = data + le16_to_cpu(rel_info->len);
>>>>>> +	/* TODO:  This needs better sanity checking I think.
>>>>>> +	 * Likely a corrupted firmware with bad rel_info->len, for instance,
>>>>>> +	 * would blow this up.
>>>>>> +	 */
>>>>>
>>>>> can you please repost dropping this comment?
>>>>
>>>> Why?  Looks to me like this portion of mtk driver logic assumes firmware is
>>>> never corrupted on accident or on purpose.  It should be fixed at some point.
>>>
>>> even if this is a theoretical issue, this does not seem the right way to track
>>> it and it is not related to this patch.
>>
>> There is no better way I know to track such problems, it is not like random OSS developers will
>> read kernel bugzilla for this sort of thing, and email bug report about it will be buried in days
>> and never seen again.
>>
>> And it is related to crazy memory corruption bugs since a corrupted firmware could cause similar
>> memory corruption.  So next time someone is working on this code, maybe they will see the
>> comment and code it more defensively.
> 
> honestly I do not think this comment is so useful to understand a memory corruption issue,
> it just makes the code uglier. If you feel this is a real issue, I would say to post a
> patch to improve the code :).

I care enough to note a complaint, but not enough to fix it.  Someone with docs about the firmware
format would have a better chance of doing a good job at this I think.

I'll post a new patch removing the comment when I get a chance to work on kernel code again.

If someone has time and interest to do it sooner, please feel free to submit a revised patch.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com