| Message ID | 20240413001031.481961-1-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | drm/i915/hwmon: Get rid of devm | expand |
Am 13.04.24 um 02:10 schrieb Ashutosh Dixit: > 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 the drvdata. However, it appears devres does not do this > consistently, so that we occasionally see drvdata being released before > hwmon itself. This results in a uaf if hwmon sysfs is accessed during > device unbind. > > The only way out of this seems to be do get rid of devm_ and release/free > everything explicitly during device unbind. Hi, could it be that the underlying cause for this is the fact that you are using devres on a DRM device? The documentation states that: devres managed resources like devm_kmalloc() can only be used for resources directly related to the underlying hardware device, and only used in code paths fully protected by drm_dev_enter() and drm_dev_exit(). That said, since the i915 driver is already removing the hwmon device manually with i915_hwmon_unregister(), i agree that not using devres in this case seems to be the solution. Thanks, Armin Wolf > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366 > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/i915_hwmon.c | 46 ++++++++++++++++++++++++------- > 1 file changed, 36 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 8c3f443c8347..5f6022b148d7 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,31 @@ 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; > + struct hwm_drvdata *ddat; > + struct intel_gt *gt; > + int i; > + > + hwmon = fetch_and_zero(&i915->hwmon); > + if (!hwmon) > + return; > + > + ddat = &hwmon->ddat; > + > + for_each_gt(gt, i915, i) { > + struct hwm_drvdata *ddat_gt; > + > + 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); > }
On Fri, 12 Apr 2024 17:10:31 -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 the drvdata. However, it appears devres does not do this > consistently, so that we occasionally see drvdata being released before > hwmon itself. This results in a uaf if hwmon sysfs is accessed during > device unbind. > > The only way out of this seems to be do get rid of devm_ and release/free > everything explicitly during device unbind. > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366 > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Please don't review this patch yet, I will send a v2 tomorrow. Please review the v2. Thanks.
On Sat, 13 Apr 2024 07:43:50 -0700, Armin Wolf wrote: > Hi Armin, > Am 13.04.24 um 02:10 schrieb Ashutosh Dixit: > > > 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 the drvdata. However, it appears devres does not do this > > consistently, so that we occasionally see drvdata being released before > > hwmon itself. This results in a uaf if hwmon sysfs is accessed during > > device unbind. > > > > The only way out of this seems to be do get rid of devm_ and release/free > > everything explicitly during device unbind. > > could it be that the underlying cause for this is the fact that you are using > devres on a DRM device? > > The documentation states that: > > devres managed resources like devm_kmalloc() can only be used for resources > directly related to the underlying hardware device, and only used in code > paths fully protected by drm_dev_enter() and drm_dev_exit(). I just posted v2 of the patch and updated https://gitlab.freedesktop.org/drm/intel/-/issues/10366. The updates do include stack traces for two separate code paths in i915 which release devres. Actually I am not sure if this is due to using devres on a DRM device. I was thinking the PCI device would be more appropriate, but looks like DRM drivers don't have the parent PCI device available in their data structs. > That said, since the i915 driver is already removing the hwmon device manually > with i915_hwmon_unregister(), Well previously i915_hwmon_unregister() was almost empty (and could actually be eliminated). > i agree that not using devres in this case seems to be the solution. Yeah that seems to me too to be the easiest way out of this situation. Thanks. -- Ashutosh
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 8c3f443c8347..5f6022b148d7 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,31 @@ 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; + struct hwm_drvdata *ddat; + struct intel_gt *gt; + int i; + + hwmon = fetch_and_zero(&i915->hwmon); + if (!hwmon) + return; + + ddat = &hwmon->ddat; + + for_each_gt(gt, i915, i) { + struct hwm_drvdata *ddat_gt; + + 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); }
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 the drvdata. However, it appears devres does not do this consistently, so that we occasionally see drvdata being released before hwmon itself. This results in a uaf if hwmon sysfs is accessed during device unbind. The only way out of this seems to be do get rid of devm_ and release/free everything explicitly during device unbind. Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366 Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 46 ++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 10 deletions(-)