linux-kernel - Re: [PATCH] wifi: mt76: mt7996: avoid potential null deref in mt7996_get_et

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <223c7280-443d-49b4-96b2-90472339dcd4@stanley.mountain>
Date: Mon, 24 Mar 2025 08:50:37 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: James Dutton <james.dutton@...il.com>
Cc: Johannes Berg <johannes@...solutions.net>,
	Markus Elfring <Markus.Elfring@....de>,
	Qasim Ijaz <qasdev00@...il.com>, linux-wireless@...r.kernel.org,
	linux-mediatek@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org,
	LKML <linux-kernel@...r.kernel.org>,
	Angelo Gioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Bo Jiao <bo.jiao@...iatek.com>, Felix Fietkau <nbd@....name>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Peter Chiu <chui-hao.chiu@...iatek.com>,
	Ryder Lee <ryder.lee@...iatek.com>,
	Sean Wang <sean.wang@...iatek.com>,
	Shayne Chen <shayne.chen@...iatek.com>
Subject: Re: [PATCH] wifi: mt76: mt7996: avoid potential null deref in
 mt7996_get_et_stats()

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