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

From: "Krzysztofik, Janusz" <janusz.krzysztofik@intel.com>
To: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"Nilawar, Badal" <badal.nilawar@intel.com>,
	"Gupta, Anshuman" <anshuman.gupta@intel.com>,
	"Vivi, Rodrigo" <rodrigo.vivi@intel.com>,
	"Roper,  Matthew D" <matthew.d.roper@intel.com>
Subject: Re: [PATCH] drm/i915/hwmon: Remove i915_hwmon_unregister() during driver unbind
Date: Thu, 28 Mar 2024 10:06:51 +0000	[thread overview]
Message-ID: <2114685.bB369e8A3T@jkrzyszt-mobl2.ger.corp.intel.com> (raw)
In-Reply-To: <85frwb5s3k.wl-ashutosh.dixit@intel.com>

Hi Ashutosh,

On Wednesday, 27 March 2024 21:37:19 CET Dixit, Ashutosh wrote:
> 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. 

OK, but the parent device has a lot of managed resources, not only hwmon, and 
for me hwmon apparently depends on at least one of those resources, e.g., a 
structure with pointers to hardware accessors or hardware registers.  Release 
of all those i915 resources is not atomic, then it may happen that a resource 
which hwmon depends on is released while the hwmon resource still active and 
has its sysfs interface exposed, I believe.

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

If that occurred the actual scenario of the failure, i.e., from still user 
reachable code of our sysfs accessors, dev_get_drvdata(dev) pointing to the 
ddat field of an already freed hwmon structure, then I would blame hwmon core 
for that, not our usage of it.

Thanks,
Janusz

> so there's
> still something we are missing.
> 
> Thanks.
> --
> Ashutosh
> 

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.