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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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