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.di...@intel.com>
> ---
> 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.v...@intel.com>
> +
> + 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
>
