Message-ID: <754bb188-2171-dd6b-d7c6-91cbca723758@candelatech.com>
Date:   Tue, 21 Mar 2023 08:00:14 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: [PATCH v2] wireless: mt76: mt7921: Fix use-after-free in fw
 features query.
Content-Language: en-MW
To:     Lorenzo Bianconi <lorenzo@kernel.org>
Cc:     linux-wireless@vger.kernel.org
References: <20230308175832.2394061-1-greearb@candelatech.com>
 <ZBl/vVj3wiaZl+du@lore-desk>
 <8f3fce81-dae7-9850-87ac-ed2b869c9dfd@candelatech.com>
 <ZBmw+uTa+CiV/KKJ@lore-desk>
 <98c35dc6-580b-d877-4124-93d98f2a163c@candelatech.com>
 <ZBnBH9oiwogHqfUt@lore-desk>
From:   Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
In-Reply-To: <ZBnBH9oiwogHqfUt@lore-desk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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