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

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

[Matthew Auld]

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>
---
 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

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

[Rodrigo Vivi]

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
> 

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

[Nilawar, Badal]

On 05-10-2023 22:08, 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: Badal Nilawar <badal.nilawar@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;