Re: [PATCH] drm/i915/hwmon: Remove i915_hwmon_unregister() during driver unbind

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

On Wed, 27 Mar 2024 02:15:27 -0700, Krzysztofik, Janusz wrote:
>

Hi Janusz,

> For me, that still doesn't explain why you think that i915->hwmon reset to
> NULL on i915 driver unregister can be the root cause of the reported UAF in
> hwmon sysfs and this patch is going to fix that UAF issue.  I can see no
> references to i915->hwmon in that code, and even then, that's not NULL what is
> reported here as non-canonical address.  The code is using a no longer valid
> pointer to an already freed (and poisoned) memory.

Correct, I said basically the same thing in my reply to the patch. That the
patch doesn't explain that ddat seems to have been freed and poisoned.

> > > I think that instead of dropping i915_hwmon_unregister() we have to actually
> > > unregister hwmon in the body of that function, which is called from
> > > i915_driver_unregister() intended for closing user interfaces, then called
> > > relatively early during driver unbind, so hwmon sysfs entries disappear before
> > > i915 structures, especially uncore used by hwmon code, are freed.
> >
> > You mean uncore are freed before hwmon get unregistered, I don't think
> > so. uncore is also drm device managed resource so in sequence I think it
> > should be freed after i915 dev managed resources freed.
>
> If both uncore and hwmon are managed resources of i915 device then how can you
> predict which of them is released first?

Look at __hwmon_device_register. Here we see:

	hdev->parent = dev

So the hwmon device is a child device of the drm device (against which ddat
is devm_kzalloc'd). Since a child device holds a reference against the
parent (device_add() has get_device(dev->parent)), I would expect the hwmon
device to disappear before the parent drm device. And I am assuming hwmon
sysfs is linked to the hwmon device, so sysfs should disappear before ddat
getting freed. But apparently this is not what is happening, so there's
still something we are missing.

Thanks.
--
Ashutosh