| Message ID | 9f5b0cf6c4296d3a9e78a95516cf26d1db4baba9.1627696765.git.ryder.lee@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] mt76: mt7915: fix hwmon temp sensor mem use-after-free | expand |
On 2021-07-31 04:17, 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. Where does the use-after-free bug come from? It's not obvious to me why using KBUILD_MODNAME instead of wiphy_name() fixes it. I still think the phy name should probably be part of the prefix. > With the change: > > mt7915-pci-1400 > Adapter: PCI adapter > temp1: +49.0°C > > Fixes: d6938251bb5b (mt76: mt7915: add thermal sensor device support) The format is wrong (missing quotes), and the hash references a commit that's not in any upstream tree. - Felix
On 8/13/21 3:15 AM, Felix Fietkau wrote: > > On 2021-07-31 04:17, 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. > Where does the use-after-free bug come from? It's not obvious to me why > using KBUILD_MODNAME instead of wiphy_name() fixes it. > I still think the phy name should probably be part of the prefix. We rename phy devices as part of our normal operation, I think maybe that helps trigger the bug. 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. But, I did not actually verify that. Thanks, Ben > >> With the change: >> >> mt7915-pci-1400 >> Adapter: PCI adapter >> temp1: +49.0°C >> >> Fixes: d6938251bb5b (mt76: mt7915: add thermal sensor device support) > The format is wrong (missing quotes), and the hash references a commit > that's not in any upstream tree. > > - Felix > -- Ben Greear <greearb@candelatech.com> Candela Technologies Inc http://www.candelatech.com
On 2021-08-13 15:54, Ben Greear wrote: > On 8/13/21 3:15 AM, Felix Fietkau wrote: >> >> On 2021-07-31 04:17, 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. >> Where does the use-after-free bug come from? It's not obvious to me why >> using KBUILD_MODNAME instead of wiphy_name() fixes it. >> I still think the phy name should probably be part of the prefix. > > We rename phy devices as part of our normal operation, I think maybe > that helps trigger the bug. > > 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. But, > I did not actually verify that. That makes sense. It seems that thermal copies the string internally, but hwmon does not. How about using devm_kstrdup on the wiphy name instead of using KBUILD_MODNAME? If you really don't want to use the initial phy name, there's also the option of using dev_name(dev->mt76.dev) - Felix
diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/init.c b/drivers/net/wireless/mediatek/mt76/mt7915/init.c index 77c7486d6a5c..a1b9e1b3f700 100644 --- a/drivers/net/wireless/mediatek/mt76/mt7915/init.c +++ b/drivers/net/wireless/mediatek/mt76/mt7915/init.c @@ -155,13 +155,13 @@ static void mt7915_unregister_thermal(struct mt7915_phy *phy) thermal_cooling_device_unregister(phy->cdev); } -static int mt7915_thermal_init(struct mt7915_phy *phy) +static int mt7915_thermal_init(struct mt7915_phy *phy, const char *prefix) { struct wiphy *wiphy = phy->mt76->hw->wiphy; struct thermal_cooling_device *cdev; struct device *hwmon; - cdev = thermal_cooling_device_register(wiphy_name(wiphy), phy, + cdev = thermal_cooling_device_register(prefix, phy, &mt7915_thermal_ops); if (!IS_ERR(cdev)) { if (sysfs_create_link(&wiphy->dev.kobj, &cdev->device.kobj, @@ -175,7 +175,7 @@ static int mt7915_thermal_init(struct mt7915_phy *phy) return 0; hwmon = devm_hwmon_device_register_with_groups(&wiphy->dev, - wiphy_name(wiphy), phy, + prefix, phy, mt7915_hwmon_groups); if (IS_ERR(hwmon)) return PTR_ERR(hwmon); @@ -403,7 +403,7 @@ static int mt7915_register_ext_phy(struct mt7915_dev *dev) if (ret) goto error; - ret = mt7915_thermal_init(phy); + ret = mt7915_thermal_init(phy, KBUILD_MODNAME "-ext"); if (ret) goto error; @@ -853,7 +853,7 @@ int mt7915_register_device(struct mt7915_dev *dev) if (ret) return ret; - ret = mt7915_thermal_init(&dev->phy); + ret = mt7915_thermal_init(&dev->phy, KBUILD_MODNAME); if (ret) return ret;