Re: [PATCH] wifi: mt76: mt7996: avoid potential null deref in mt7996_get_et_stats()

[Dan Carpenter <dan.carpenter@linaro.org>]

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