[PATCH] drm/i915/hwmon: Get rid of devm

[Armin Wolf]

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 at 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);
>   }