On Fri, 22 Mar 2024, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Fri, Mar 22, 2024 at 07:54:03PM +0100, Janusz Krzysztofik wrote:
>> Hwmon is registered as a managed resource of i915. Its functionality
>> depends of availability of i915 uncore.
>
> Instead of polluting all code with this junk I think
> either fixing the devm stuff, or just getting rid of it
> if it's unfixable would be preferable.
Yeah, ugh.
The worst part about sprinkling the checks around is actually that
people *will* cargo cult copy-paste them elsewhere, because they have no
confidence about the lifetimes of objects, and do this just to be
sure. And then we'll have these checks all over the place for no reason.
BR,
Jani.
>
>>
>> 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@xxxxxxxxx>
>> Fixes: b3b088e28183 ("drm/i915/hwmon: Add HWMON infrastructure")
>> Closes: https://gitlab.freedesktop.org/drm/intel/issues/10366
>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx>
>> Cc: Dale B Stimson <dale.b.stimson@xxxxxxxxx>
>> Cc: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
>> Cc: Riana Tauro <riana.tauro@xxxxxxxxx>
>> Cc: <stable@xxxxxxxxxxxxxxx> # 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
--
Jani Nikula, Intel
