On Mon, Apr 15, 2024 at 03:36:12PM -0700, Ashutosh Dixit wrote: > When both hwmon and hwmon drvdata (on which hwmon depends) are device > managed resources, the expectation, on device unbind, is that hwmon will be > released before drvdata. However, in i915 there are two separate code > paths, which both release either drvdata or hwmon and either can be > released before the other. These code paths (for device unbind) are as > follows (see also the bug referenced below): > > Call Trace: > release_nodes+0x11/0x70 > devres_release_group+0xb2/0x110 > component_unbind_all+0x8d/0xa0 > component_del+0xa5/0x140 > intel_pxp_tee_component_fini+0x29/0x40 [i915] > intel_pxp_fini+0x33/0x80 [i915] > i915_driver_remove+0x4c/0x120 [i915] > i915_pci_remove+0x19/0x30 [i915] > pci_device_remove+0x32/0xa0 > device_release_driver_internal+0x19c/0x200 > unbind_store+0x9c/0xb0 > > and > > Call Trace: > release_nodes+0x11/0x70 > devres_release_all+0x8a/0xc0 > device_unbind_cleanup+0x9/0x70 > device_release_driver_internal+0x1c1/0x200 > unbind_store+0x9c/0xb0 > > This means that in i915, if use devm, we cannot gurantee that hwmon will > always be released before drvdata. Which means that we have a uaf if hwmon > sysfs is accessed when drvdata has been released but hwmon hasn't. > > The only way out of this seems to be do get rid of devm_ and release/free > everything explicitly during device unbind. > > v2: Change commit message and other minor code changes > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366 > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_hwmon.c | 41 +++++++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 8c3f443c8347..46c24b1ee6df 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -792,7 +792,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > if (!IS_DGFX(i915)) > return; > > - hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL); > + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL); > if (!hwmon) > return; > > @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) > hwm_get_preregistration_info(i915); > > /* hwmon_dev points to device hwmon<i> */ > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name, > - ddat, > - &hwm_chip_info, > - hwm_groups); > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name, > + ddat, > + &hwm_chip_info, > + hwm_groups); > if (IS_ERR(hwmon_dev)) { > i915->hwmon = NULL; > return; > @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) > if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0)) > continue; > > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name, > - ddat_gt, > - &hwm_gt_chip_info, > - NULL); > + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name, > + ddat_gt, > + &hwm_gt_chip_info, > + NULL); > if (!IS_ERR(hwmon_dev)) > ddat_gt->hwmon_dev = hwmon_dev; > } > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > void i915_hwmon_unregister(struct drm_i915_private *i915) > { > - fetch_and_zero(&i915->hwmon); > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon); > + struct hwm_drvdata *ddat = &hwmon->ddat; > + struct intel_gt *gt; > + int i; > + > + if (!hwmon) > + return; "that's too late", we are going to hear from static analyzer tools. beter to move ddat = &hwmon->ddat; after this return. with that, Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > + > + for_each_gt(gt, i915, i) { > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i; > + > + if (ddat_gt->hwmon_dev) { > + hwmon_device_unregister(ddat_gt->hwmon_dev); > + ddat_gt->hwmon_dev = NULL; > + } > + } > + > + if (ddat->hwmon_dev) > + hwmon_device_unregister(ddat->hwmon_dev); > + > + mutex_destroy(&hwmon->hwmon_lock); > + kfree(hwmon); > } > -- > 2.41.0 >
