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

[Ben Greear]

On 9/1/21 10:49 AM, Ryder Lee wrote:
> From: Ben Greear <greearb at 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 at candelatech.com>
> Signed-off-by: Ryder Lee <ryder.lee at 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.

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_groups);
>   	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.

Thanks,
Ben

>   	bf->bw = sta->bandwidth;
>   	bf->ibf_dbw = sta->bandwidth;
>   	bf->ibf_nrow = tx_ant;
> 


-- 
Ben Greear <greearb at candelatech.com>
Candela Technologies Inc  http://www.candelatech.com