[PATCH] drm/i915/hwmon: Fix potential UAF on driver unbind

* [PATCH] drm/i915/hwmon: Fix potential UAF on driver unbind
@ 2024-03-22 18:54 Janusz Krzysztofik
  2024-03-22 20:43 ` Ville Syrjälä
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Janusz Krzysztofik @ 2024-03-22 18:54 UTC (permalink / raw)
  To: intel-gfx
  Cc: dri-devel, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Tvrtko Ursulin, Ashutosh Dixit, Dale B Stimson, Anshuman Gupta,
	Riana Tauro, Andi Shyti, Janusz Krzysztofik

Hwmon is registered as a managed resource of i915.  Its functionality
depends of availability of i915 uncore.

On i915 unbind, we now block i915 access to hwmon, but leave i915 uncore
still available to hwmon.  Since uncore structures may be freed before
hwmon is actually unregistered, that may lead to UAF if someone is still
using hwmon sysfs interface:

<7> [469.045388] i915 0000:03:00.0: [drm] intel_gt_set_wedged called from intel_gt_set_wedged_on_fini+0xd/0x30 [i915]
<7> [469.120600] i915 0000:03:00.0: [drm:drm_client_release] drm_fb_helper
<4> [469.188183] general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6dbf: 0000 [#1] PREEMPT SMP NOPTI
<4> [469.198970] CPU: 4 PID: 451 Comm: prometheus-node Tainted: G     U             6.8.0-CI_DRM_14465-g198e9d7559f7+ #1
<4> [469.209321] Hardware name: Intel Corporation Raptor Lake Client Platform/RPL-S ADP-S DDR5 UDIMM CRB, BIOS RPLSFWI1.R00.4221.A00.2305271351 05/27/2023
<4> [469.222589] RIP: 0010:hwm_energy+0x2b/0x100 [i915]
<4> [469.227527] Code: 48 89 e5 41 57 41 56 41 55 41 54 53 48 89 fb 48 83 e4 f0 48 83 ec 10 4c 8b 77 08 4c 8b 2f 8b 7f 34 48 89 74 24 08 85 ff 78 2b <45> 8b bd 54 02 00 00 49 8b 7e 18 e8 35 df ea ff 49 89 c4 48 85 c0
<4> [469.246119] RSP: 0018:ffffc90007aa7d00 EFLAGS: 00010202
<4> [469.251314] RAX: 0000000000000000 RBX: ffff88812b4b5828 RCX: 0000000000000000
<4> [469.258396] RDX: 0000000000000001 RSI: ffffc90007aa7da0 RDI: 000000006b6b6b6b
<4> [469.265478] RBP: ffffc90007aa7d40 R08: ffffc90007aa7da0 R09: 0000000000000001
<4> [469.272559] R10: 0000000000000001 R11: ffff88811889d340 R12: ffff88810e088810
<4> [469.279641] R13: 6b6b6b6b6b6b6b6b R14: 6b6b6b6b6b6b6b6b R15: ffff88812b4b5828
<4> [469.286722] FS:  00007f7b9c800700(0000) GS:ffff88888d000000(0000) knlGS:0000000000000000
<4> [469.294749] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4> [469.300454] CR2: 0000561a5fb45d88 CR3: 0000000111ff0000 CR4: 0000000000f50ef0
<4> [469.307535] PKRU: 55555554
<4> [469.310237] Call Trace:
<4> [469.312678]  <TASK>
<4> [469.335396]  hwm_read+0x9a/0x310 [i915]
<4> [469.339342]  hwmon_attr_show+0x36/0x120
<4> [469.343163]  dev_attr_show+0x15/0x60
<4> [469.346723]  sysfs_kf_seq_show+0xb5/0x100

Reset pointers to uncore in hwmon when unregistering i915, and teach hwmon
sysfs accessors to start with checking validity of those pointers and fail
immediately if no longer valid.

Suggested-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Fixes: b3b088e28183 ("drm/i915/hwmon: Add HWMON infrastructure")
Closes: https://gitlab.freedesktop.org/drm/intel/issues/10366
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Dale B Stimson <dale.b.stimson@intel.com>
Cc: Anshuman Gupta <anshuman.gupta@intel.com>
Cc: Riana Tauro <riana.tauro@intel.com>
Cc: <stable@vger.kernel.org> # v6.2+
---
 drivers/gpu/drm/i915/i915_hwmon.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index b758fd110c204..f5c1d04dae054 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -619,6 +619,9 @@ hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 {
 	struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata;
 
+	if (!ddat->uncore)
+		return 0;
+
 	switch (type) {
 	case hwmon_in:
 		return hwm_in_is_visible(ddat, attr);
@@ -639,6 +642,9 @@ hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 {
 	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
 
+	if (!ddat->uncore)
+		return -ENXIO;
+
 	switch (type) {
 	case hwmon_in:
 		return hwm_in_read(ddat, attr, val);
@@ -659,6 +665,9 @@ hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 {
 	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
 
+	if (!ddat->uncore)
+		return -ENXIO;
+
 	switch (type) {
 	case hwmon_power:
 		return hwm_power_write(ddat, attr, channel, val);
@@ -686,6 +695,9 @@ hwm_gt_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 {
 	struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata;
 
+	if (!ddat->uncore)
+		return 0;
+
 	switch (type) {
 	case hwmon_energy:
 		return hwm_energy_is_visible(ddat, attr);
@@ -700,6 +712,9 @@ hwm_gt_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 {
 	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
 
+	if (!ddat->uncore)
+		return -ENXIO;
+
 	switch (type) {
 	case hwmon_energy:
 		return hwm_energy_read(ddat, attr, val);
@@ -850,5 +865,14 @@ void i915_hwmon_register(struct drm_i915_private *i915)
 
 void i915_hwmon_unregister(struct drm_i915_private *i915)
 {
+	struct i915_hwmon *hwmon = i915->hwmon;
+	struct intel_gt *gt;
+	int i;
+
+	for_each_gt(gt, i915, i)
+		fetch_and_zero(&hwmon->ddat_gt[i].uncore);
+
+	fetch_and_zero(&hwmon->ddat.uncore);
+
 	fetch_and_zero(&i915->hwmon);
 }
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread