On 27-03-2024 02:58, Krzysztofik, Janusz wrote:
On Tuesday, 26 March 2024 13:48:38 CET Badal Nilawar wrote:
i915_hwmon and its resources are managed resources of i915 dev.
During i915 driver unregister flow the function i915_hwmon_unregister()
explicitly makes i915_hwmon resource NULL. This happen before
hwmon is actually unregistered. Doing so may cause UAF if hwmon
sysfs is being accessed:
<7> [518.386591] i915 0000:03:00.0: [drm] intel_gt_set_wedged called from intel_gt_set_wedged_on_fini+0xd/0x30 [i915]
<7> [518.471128] i915 0000:03:00.0: [drm:drm_client_release] drm_fb_helper
<4> [518.501476] general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6dbf: 0000 [#1] PREEMPT SMP NOPTI
<4> [518.512264] CPU: 6 PID: 679 Comm: prometheus-node Tainted: G U 6.9.0-rc1-CI_DRM_14482-g4a8fabcf2f1a+ #1
<4> [518.522951] 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> [518.536217] RIP: 0010:hwm_energy+0x2b/0x100 [i915]
<4> [518.541159] 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 e4 ea ff 49 89 c4 48 85 c0
<4> [518.559746] RSP: 0018:ffffc900077efd00 EFLAGS: 00010202
<4> [518.564943] RAX: 0000000000000000 RBX: ffff8881e3078428 RCX: 0000000000000000
<4> [518.572025] RDX: 0000000000000001 RSI: ffffc900077efda0 RDI: 000000006b6b6b6b
<4> [518.579107] RBP: ffffc900077efd40 R08: ffffc900077efda0 R09: 0000000000000001
<4> [518.586186] R10: 0000000000000001 R11: ffff88810c19aa00 R12: ffff888243e1a010
<4> [518.593264] R13: 6b6b6b6b6b6b6b6b R14: 6b6b6b6b6b6b6b6b R15: ffff8881e3078428
<4> [518.600343] FS: 00007f9def400700(0000) GS:ffff88888d100000(0000) knlGS:0000000000000000
<4> [518.608373] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4> [518.614077] CR2: 0000564f19bff288 CR3: 0000000119f94000 CR4: 0000000000f50ef0
<4> [518.621158] PKRU: 55555554
<4> [518.623858] Call Trace:
<4> [518.626303] <TASK>
<4> [518.628400] ? __die_body+0x1a/0x60
<4> [518.631881] ? die_addr+0x38/0x60
<4> [518.635182] ? exc_general_protection+0x1a1/0x400
<4> [518.639862] ? asm_exc_general_protection+0x26/0x30
<4> [518.644710] ? hwm_energy+0x2b/0x100 [i915]
<4> [518.649007] hwm_read+0x9a/0x310 [i915]
<4> [518.652953] hwmon_attr_show+0x36/0x140
<4> [518.656775] dev_attr_show+0x15/0x60
I don't think that's a good example of what you are talking about in your
commit description. I haven't looked throughout the i915 code to find out who
actually uses that i915->hwmon pointer and when, but while looking at the
hwmon code that now fails on sysfs access, I haven't noticed any use of
i915->hwmon.
i915_hwmon is main structure and I think issue is ddat is invalid.
struct hwm_drvdata {
struct i915_hwmon *hwmon;
struct intel_uncore *uncore;
struct device *hwmon_dev;
struct hwm_energy_info ei; /* Energy info for energy1_input */
char name[12];
int gt_n;
bool reset_in_progress;
wait_queue_head_t waitq;
};
struct i915_hwmon {
struct hwm_drvdata ddat;
struct hwm_drvdata ddat_gt[I915_MAX_GT];
struct mutex hwmon_lock; /* counter overflow logic and rmw */
struct hwm_reg rg;
int scl_shift_power;
int scl_shift_energy;
int scl_shift_time;
};
(gdb) list *hwm_energy+0x2b
0x161f8b is in hwm_energy (drivers/gpu/drm/i915/i915_hwmon.c:134).
129 struct hwm_energy_info *ei = &ddat->ei;
130 intel_wakeref_t wakeref;
131 i915_reg_t rgaddr;
132 u32 reg_val;
133
134 if (ddat->gt_n >= 0)
135 rgaddr = hwmon->rg.energy_status_tile;
136 else
137 rgaddr = hwmon->rg.energy_status_all;
138
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.
871 static int intel_gt_tile_setup(struct intel_gt *gt, phys_addr_t
phys_addr)
872 {
873 int ret;
874
875 if (!gt_is_root(gt)) {
876 struct intel_uncore *uncore;
877 spinlock_t *irq_lock;
878
879 uncore = drmm_kzalloc(>->i915->drm,
sizeof(*uncore), GFP_KERNEL);
880 if (!uncore)
881 return -ENOMEM;
882
883 irq_lock = drmm_kzalloc(>->i915->drm,
sizeof(*irq_lock), GFP_KERNEL);
884 if (!irq_lock)
885 return -ENOMEM;
886
Regards,
Badal
Thanks,
Janusz
Fixes: b3b088e28183 ("drm/i915/hwmon: Add HWMON infrastructure")
Closes: https://gitlab.freedesktop.org/drm/intel/issues/10366
Cc: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
Cc: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx>
Signed-off-by: Badal Nilawar <badal.nilawar@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_driver.c | 2 --
drivers/gpu/drm/i915/i915_hwmon.c | 5 -----
2 files changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 4b9233c07a22..a95b455964b7 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -660,8 +660,6 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
for_each_gt(gt, dev_priv, i)
intel_gt_driver_unregister(gt);
- i915_hwmon_unregister(dev_priv);
-
i915_perf_unregister(dev_priv);
i915_pmu_unregister(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index c9169e56b9a1..91f171752d34 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -841,8 +841,3 @@ void i915_hwmon_register(struct drm_i915_private *i915)
ddat_gt->hwmon_dev = hwmon_dev;
}
}
-
-void i915_hwmon_unregister(struct drm_i915_private *i915)
-{
- fetch_and_zero(&i915->hwmon);
-}
