From: Ashutosh Dixit <ashutosh.dixit@intel.com>
To: intel-gfx@lists.freedesktop.org
Cc: "Badal Nilawar" <badal.nilawar@intel.com>,
"Andi Shyti" <andi.shyti@intel.com>,
"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
linux-hwmon@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: [PATCH v2] drm/i915/hwmon: Get rid of devm
Date: Mon, 15 Apr 2024 15:36:12 -0700 [thread overview]
Message-ID: <20240415223612.738535-1-ashutosh.dixit@intel.com> (raw)
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 drvdata. However, in i915 there are two separate code
paths, which both release either drvdata or hwmon and either can be
released before the other. These code paths (for device unbind) are as
follows (see also the bug referenced below):
Call Trace:
release_nodes+0x11/0x70
devres_release_group+0xb2/0x110
component_unbind_all+0x8d/0xa0
component_del+0xa5/0x140
intel_pxp_tee_component_fini+0x29/0x40 [i915]
intel_pxp_fini+0x33/0x80 [i915]
i915_driver_remove+0x4c/0x120 [i915]
i915_pci_remove+0x19/0x30 [i915]
pci_device_remove+0x32/0xa0
device_release_driver_internal+0x19c/0x200
unbind_store+0x9c/0xb0
and
Call Trace:
release_nodes+0x11/0x70
devres_release_all+0x8a/0xc0
device_unbind_cleanup+0x9/0x70
device_release_driver_internal+0x1c1/0x200
unbind_store+0x9c/0xb0
This means that in i915, if use devm, we cannot gurantee that hwmon will
always be released before drvdata. Which means that we have a uaf if hwmon
sysfs is accessed when drvdata has been released but hwmon hasn't.
The only way out of this seems to be do get rid of devm_ and release/free
everything explicitly during device unbind.
v2: Change commit message and other minor code changes
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
drivers/gpu/drm/i915/i915_hwmon.c | 41 +++++++++++++++++++++++--------
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index 8c3f443c8347..46c24b1ee6df 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -792,7 +792,7 @@ void i915_hwmon_register(struct drm_i915_private *i915)
if (!IS_DGFX(i915))
return;
- hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
+ hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
if (!hwmon)
return;
@@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
hwm_get_preregistration_info(i915);
/* hwmon_dev points to device hwmon<i> */
- hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
- ddat,
- &hwm_chip_info,
- hwm_groups);
+ hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
+ ddat,
+ &hwm_chip_info,
+ hwm_groups);
if (IS_ERR(hwmon_dev)) {
i915->hwmon = NULL;
return;
@@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0))
continue;
- hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name,
- ddat_gt,
- &hwm_gt_chip_info,
- NULL);
+ hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name,
+ ddat_gt,
+ &hwm_gt_chip_info,
+ NULL);
if (!IS_ERR(hwmon_dev))
ddat_gt->hwmon_dev = hwmon_dev;
}
@@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915)
void i915_hwmon_unregister(struct drm_i915_private *i915)
{
- fetch_and_zero(&i915->hwmon);
+ struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon);
+ struct hwm_drvdata *ddat = &hwmon->ddat;
+ struct intel_gt *gt;
+ int i;
+
+ if (!hwmon)
+ return;
+
+ for_each_gt(gt, i915, i) {
+ struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i;
+
+ if (ddat_gt->hwmon_dev) {
+ hwmon_device_unregister(ddat_gt->hwmon_dev);
+ ddat_gt->hwmon_dev = NULL;
+ }
+ }
+
+ if (ddat->hwmon_dev)
+ hwmon_device_unregister(ddat->hwmon_dev);
+
+ mutex_destroy(&hwmon->hwmon_lock);
+ kfree(hwmon);
}
--
2.41.0
next reply other threads:[~2024-04-15 22:36 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-15 22:36 Ashutosh Dixit [this message]
2024-04-15 23:35 ` [PATCH v2] drm/i915/hwmon: Get rid of devm Armin Wolf
2024-04-16 4:05 ` Dixit, Ashutosh
2024-04-16 7:44 ` Jani Nikula
2024-04-16 18:55 ` Rodrigo Vivi
2024-04-16 19:02 ` Dixit, Ashutosh
2024-04-16 19:15 ` Rodrigo Vivi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240415223612.738535-1-ashutosh.dixit@intel.com \
--to=ashutosh.dixit@intel.com \
--cc=andi.shyti@intel.com \
--cc=badal.nilawar@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).