Message-ID: <8f3fce81-dae7-9850-87ac-ed2b869c9dfd@candelatech.com>
Date:   Tue, 21 Mar 2023 06:20:13 -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>
From:   Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
In-Reply-To: <ZBl/vVj3wiaZl+du@lore-desk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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