Re: [Intel-xe] [PATCH] drm/xe/hwmon: fix uaf on unload

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Oct 05, 2023 at 05:38:55PM +0100, Matthew Auld wrote:
> It doesn't look like you can mix and match devm_ and drmmm_ for a
> managed resource. For drmmm the resources are all tracked in drm with
> its own list, and there is only one devm_ resource for the entire list.
> If the driver itself also adds some of its own devm resources, then
> those will be released first. In the case of hwmon the devm_kzalloc will
> be freed before the drmmm_ action to destroy the mutex allocated within,
> leading to uaf.
> 
> Since hwmon itself wants to use devm, rather use that for the mutex
> destroy.
> 
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/766
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Badal Nilawar <badal.nilawar@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_hwmon.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 1deb5007e1e2..9d3e06b96073 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -584,6 +584,13 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe)
>  		xe_hwmon_energy_get(hwmon, &energy);
>  }
>  
> +static void xe_hwmon_mutex_destroy(void *arg)
> +{
> +	struct xe_hwmon *hwmon = arg;
> +
> +	mutex_destroy(&hwmon->hwmon_lock);
> +}
> +
>  void xe_hwmon_register(struct xe_device *xe)
>  {
>  	struct device *dev = xe->drm.dev;
> @@ -599,7 +606,9 @@ void xe_hwmon_register(struct xe_device *xe)
>  
>  	xe->hwmon = hwmon;
>  
> -	drmm_mutex_init(&xe->drm, &hwmon->hwmon_lock);
> +	mutex_init(&hwmon->hwmon_lock);
> +	if (devm_add_action_or_reset(dev, xe_hwmon_mutex_destroy, hwmon))
> +		return;
>  
>  	/* primary GT to access device level properties */
>  	hwmon->gt = xe->tiles[0].primary_gt;
> -- 
> 2.41.0
>