| Message ID | 20250322141910.4461-1-qasdev00@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: mt76: mt7996: avoid potential null deref in mt7996_get_et_stats() | expand |
> Ensure phy->mib is 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? … > +++ b/drivers/net/wireless/mediatek/mt76/mt7996/main.c … > @@ -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
On Sat, 2025-03-22 at 15:55 +0100, Markus Elfring wrote: > > Ensure phy->mib is 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:
diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/main.c
b/drivers/net/wireless/mediatek/mt76/mt7996/main.c
index 66575698aef1..88e013577c0d 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7996/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7996/main.c
@@ -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
On Sun, Mar 23, 2025 at 11:59:45AM +0000, James Dutton wrote: > As a security side note in relation to the following patch: > diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/main.c > b/drivers/net/wireless/mediatek/mt76/mt7996/main.c > index 66575698aef1..88e013577c0d 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt7996/main.c > +++ b/drivers/net/wireless/mediatek/mt76/mt7996/main.c > @@ -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. 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 > 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
> 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
On Mon, Mar 24, 2025 at 08:33:39AM +0100, Markus Elfring wrote: > > 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
diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/main.c b/drivers/net/wireless/mediatek/mt76/mt7996/main.c index 88e013577c0d..19391966ee3e 100644 --- a/drivers/net/wireless/mediatek/mt76/mt7996/main.c +++ b/drivers/net/wireless/mediatek/mt76/mt7996/main.c @@ -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);
Ensure phy->mib is only accessed after the null sanity check for phy otherwise the code may trigger a potential null deref. Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> --- drivers/net/wireless/mediatek/mt76/mt7996/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)