wifi: mt76: mt7996: avoid potential null deref in mt7996_get_et_stats()
Subsystems (2)
PATCH 1/1 wifi: mt76: mt7996: avoid potential null deref in mt7996_get_et_stats()
Ensure phy->mib is only accessed after the null sanity check for phy otherwise the code may trigger a potential null deref.
--- drivers/net/wireless/mediatek/mt76/mt7996/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/net/wireless/mediatek/mt76/mt7996/main.c +3 -1
@@ -1875,7 +1875,7 @@ void mt7996_get_et_stats(struct ieee80211_hw *hw,struct mt7996_dev *dev =mt7996_hw_dev (hw);struct mt7996_vif *mvif = (struct mt7996_vif *)vif->drv_priv;struct mt7996_phy *phy =mt7996_vif_link_phy (&mvif->deflink); -struct mt76_mib_stats *mib = &phy->mib; +struct mt76_mib_stats *mib;struct mt76_ethtool_worker_info wi = { .data = data, .idx = mvif->deflink.mt76.idx,
@@ -1886,6 +1886,8 @@ void mt7996_get_et_stats(struct ieee80211_hw *hw,if (!phy)return ; + mib = &phy->mib; +mutex_lock (&dev->mt76.mutex);mt7996_mac_update_stats (phy);
Signature · 1 line
2.39.5
Quoted 2 lines
Ensure
phy->mibis only accessed after the null sanity check for phy otherwise the code may trigger a potential null deref.
* Would you like to use the term “null pointer dereference” consistently?
* Were any known source code analysis tools involved also for this software improvement?
…
Quoted 1 line
+++
b/drivers/net/wireless/mediatek/mt76/mt7996/main.c
…
drivers/net/wireless/mediatek/mt76/mt7996/main.c void mt7996_get_et_stats(struct ieee80211_hw *hw, · line 1893
Quoted 9 lines
drivers/net/wireless/mediatek/mt76/mt7996/main.c +2
-0@@ -1886,6 +1886,8 @@ void mt7996_get_et_stats(struct ieee80211_hw *hw,if (!phy)return ; + mib = &phy->mib; +mutex_lock (&dev->mt76.mutex);mt7996_mac_update_stats (phy);
I suggest to move such an assignment statement directly before the place where this variable is used finally.
Regards,
Markus
Quoted 8 lines · Markus Elfring
Quoted 2 lines
Ensure
phy->mibis only accessed after the null sanity check for phy otherwise the code may trigger a potential null deref.* Would you like to use the term “null pointer dereference” consistently?
* Were any known source code analysis tools involved also for this software improvement?
Do you really have to double down? STOP. You're just wasting everyone's time.
johannes
As a security side note in relation to the following patch:
(quoted diff) +0 -0
b/drivers/net/wireless/mediatek/mt76/mt7996/main.c index 66575698aef1..88e013577c0d 100644
drivers/net/wireless/mediatek/mt76/mt7996/main.c +3 -1
@@ -68,11 +68,13 @@ static int mt7996_start(struct ieee80211_hw *hw)static void mt7996_stop_phy (struct mt7996_phy *phy) { -struct mt7996_dev *dev = phy->dev; +struct mt7996_dev *dev;if (!phy || !test_bit (MT76_STATE_RUNNING , &phy->mt76->state))return ; + dev = phy->dev; +cancel_delayed_work_sync (&phy->mt76->mac_work);mutex_lock (&dev->mt76.mutex);
Prior to that patch, the code looks like this:
static void mt7996_stop_phy(struct mt7996_phy *phy)
{
struct mt7996_dev *dev = phy->dev;
if (!phy || !test_bit(MT76_STATE_RUNNING, &phy->mt76->state))
return;The compiler will completely remove the !phy check entirely because of the use above it, so it being present in the source code is completely bogus.
If one actually needs a !phy check to be present in the compiled code, one must arrange it as per the patch above.
The fact that the !phy check is in the source code, implies to me that someone, in the past, thought it was necessary, but I think an opinion could be taken that it is there to obfuscate a security vulnerability.
Kind Regards
James
Quoted 33 lines · James Dutton
As a security side note in relation to the following patch:
drivers/net/wireless/mediatek/mt76/mt7996/main.c +3
-1@@ -68,11 +68,13 @@ static int mt7996_start(struct ieee80211_hw *hw)static void mt7996_stop_phy (struct mt7996_phy *phy) { -struct mt7996_dev *dev = phy->dev; +struct mt7996_dev *dev;if (!phy || !test_bit (MT76_STATE_RUNNING , &phy->mt76->state))return ; + dev = phy->dev; +cancel_delayed_work_sync (&phy->mt76->mac_work);mutex_lock (&dev->mt76.mutex);Prior to that patch, the code looks like this:
static voidmt7996_stop_phy(struct mt7996_phy *phy){ struct mt7996_dev *dev = phy->dev; if (!phy || !test_bit(MT76_STATE_RUNNING, &phy->mt76->state)) return;
Quoted 3 lines
The compiler will completely remove the !phy check entirely because of the use above it, so it being present in the source code is completely bogus.
No, in the kernel we use the -fno-delete-null-pointer-checks so the NULL check will always be there.
Also the "phy" point will never be NULL so the check should be removed.
regards,
dan carpenter
Quoted 10 lines
If one actually needs a !phy check to be present in the compiled code, one must arrange it as per the patch above.
The fact that the !phy check is in the source code, implies to me that someone, in the past, thought it was necessary, but I think an opinion could be taken that it is there to obfuscate a security vulnerability.
Kind Regards
James
Quoted 1 line
Also the "phy" point will never be NULL so the check should be removed.
How many tools can help to determine such a software aspect with inter-procedural analyses?
Regards,
Markus
Quoted 4 lines · Markus Elfring
Quoted 1 line
Also the "phy" point will never be NULL so the check should be removed.
How many tools can help to determine such a software aspect with inter-procedural analyses?
You can just review the code. There is only one caller.
Btw, it's fine to have unnecessary NULL checks so long as they're done consistently. Generally, we prefer people not add unnecessary code, but if it makes you feel safer, most maintainers aren't going to nit-pick you about it. If you are doing the work then you get some say your own code.
regards,
dan carpenter