| Message ID | 20230308010026.2206775-1-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Felix Fietkau |
| Headers | show |
| Series | wireless: mt76: mt7921: Fix use-after-free in fw features query. | expand |
> 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 Hi Ben, thx for fixing it. Just few minor comments inline. Regards, Lorenzo > ... > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > drivers/net/wireless/mediatek/mt76/mt7921/init.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c > index 38d6563cb12f..3b6fb1a6b351 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c > @@ -171,6 +171,7 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm) > const struct firmware *fw; > int ret, i, offset = 0; > const u8 *data, *end; > + u8 rv = 0; I would say we can use a more meaningful name here, something like offload_capa or similar. Moreover I thin you can move features pointer just in the if (rel_info->tag == MT7921_FW_TAG_FEATURE) block. Regards, Lorenzo > > ret = request_firmware(&fw, fw_wm, dev); > if (ret) > @@ -197,12 +198,17 @@ 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) { > features = (struct mt7921_fw_features *)data; > + rv = features->data; > break; > } > > @@ -211,7 +217,7 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm) > > release_firmware(fw); > > - return features ? features->data : 0; > + return rv; > } > EXPORT_SYMBOL_GPL(mt7921_check_offload_capability); > > -- > 2.39.1 >
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c index 38d6563cb12f..3b6fb1a6b351 100644 --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c @@ -171,6 +171,7 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm) const struct firmware *fw; int ret, i, offset = 0; const u8 *data, *end; + u8 rv = 0; ret = request_firmware(&fw, fw_wm, dev); if (ret) @@ -197,12 +198,17 @@ 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) { features = (struct mt7921_fw_features *)data; + rv = features->data; break; } @@ -211,7 +217,7 @@ u8 mt7921_check_offload_capability(struct device *dev, const char *fw_wm) release_firmware(fw); - return features ? features->data : 0; + return rv; } EXPORT_SYMBOL_GPL(mt7921_check_offload_capability);