Re: [PATCH v5 1/2] mt76: mt7915: fix hwmon temp sensor mem use-after-free

[Ryder Lee <ryder.lee@mediatek.com>]

On Wed, 2021-09-01 at 10:55 -0700, Ben Greear wrote:
> On 9/1/21 10:49 AM, Ryder Lee wrote:
> > From: Ben Greear <greearb@candelatech.com>
> > 
> > Without this change, garbage is seen in the hwmon name and sensors
> > output
> > for mt7915 is garbled. It appears that the hwmon logic does not
> > make a
> > copy of the incoming string, but instead just copies a char* and
> > expects
> > it to never go away.
> > 
> > Fixes: d6938251bb5b ("mt76: mt7915: add thermal sensor device
> > support")
> > Signed-off-by: Ben Greear <greearb@candelatech.com>
> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> > v5:  Use devm_kstrdup on the wiphy name as suggested.
> 
> I don't care a great deal either way, but phyname can change (which
> was original
> way to reproduce this corruption bug), so with this v5 change, then
> the hwmon 'id' could confusingly
> be 'phy0' while user has renamed the phy0 to wiphy0 or whatever.
> 
> It won't break my usage either way, so if you are happy with this,
> then good
> enough for me.

I thought of this ... Considering the hwmon can't never know the
phyname once it got changed. We can only specifiy a static "mt7915-xxx" 
to name it, or just use an initial phyname. Anyhow it's not the real
phyname you change actually.

> But, see below, there is spurious change...
> 
> 
> > v4:  Simplify flow.
> > v3:  Add 'fixes' tag to aid backports.
> > ---
> >   drivers/net/wireless/mediatek/mt76/mt7915/init.c | 8 ++++----
> >   drivers/net/wireless/mediatek/mt76/mt7915/mcu.c  | 2 +-
> >   2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/init.c
> > b/drivers/net/wireless/mediatek/mt76/mt7915/init.c
> > index acc83e9f409b..78b9abbe63f3 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7915/init.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7915/init.c
> > @@ -160,9 +160,10 @@ static int mt7915_thermal_init(struct
> > mt7915_phy *phy)
> >   	struct wiphy *wiphy = phy->mt76->hw->wiphy;
> >   	struct thermal_cooling_device *cdev;
> >   	struct device *hwmon;
> > +	const char *name;
> >   
> > -	cdev = thermal_cooling_device_register(wiphy_name(wiphy), phy,
> > -					       &mt7915_thermal_ops);
> > +	name = devm_kstrdup(&wiphy->dev, wiphy_name(wiphy),
> > GFP_KERNEL);
> > +	cdev = thermal_cooling_device_register(name, phy,
> > &mt7915_thermal_ops);
> >   	if (!IS_ERR(cdev)) {
> >   		if (sysfs_create_link(&wiphy->dev.kobj, &cdev-
> > >device.kobj,
> >   				      "cooling_device") < 0)
> > @@ -174,8 +175,7 @@ static int mt7915_thermal_init(struct
> > mt7915_phy *phy)
> >   	if (!IS_REACHABLE(CONFIG_HWMON))
> >   		return 0;
> >   
> > -	hwmon = devm_hwmon_device_register_with_groups(&wiphy->dev,
> > -						       wiphy_name(wiphy
> > ), phy,
> > +	hwmon = devm_hwmon_device_register_with_groups(&wiphy->dev,
> > name, phy,
> >   						       mt7915_hwmon_gro
> > ups);
> >   	if (IS_ERR(hwmon))
> >   		return PTR_ERR(hwmon);
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
> > b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
> > index 932cf5a629db..219bb353b56d 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7915/mcu.c
> > @@ -1962,7 +1962,7 @@ mt7915_mcu_sta_bfer_tlv(struct mt7915_dev
> > *dev, struct sk_buff *skb,
> >   	else
> >   		return;
> >   
> > -	bf->bf_cap = BIT(!ebf && dev->ibf);
> > +	bf->bf_cap = ebf ? ebf : dev->ibf << 1;
> 
> And this should not be in this patch.
> 

Oops. I messed patch up while merging to other series. Will fix.

Ryder
>