[PATCH] drm/i915/hwmon: Remove i915_hwmon_unregister() during driver unbind
Nilawar, Badal
badal.nilawar at intel.com
Wed Mar 27 04:30:17 UTC 2024
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 at intel.com>
>> Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar at intel.com>
>> ---
>> 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);
>> -}
>>
>
More information about the Intel-gfx
mailing list