From: "Nilawar, Badal" <badal.nilawar@intel.com>
To: "Krzysztofik, Janusz" <janusz.krzysztofik@intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>
Cc: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>,
"Gupta, Anshuman" <anshuman.gupta@intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel.com>,
Matt Roper <matthew.d.roper@intel.com>
Subject: Re: [PATCH] drm/i915/hwmon: Remove i915_hwmon_unregister() during driver unbind
Date: Wed, 27 Mar 2024 10:00:17 +0530 [thread overview]
Message-ID: <b132ded3-bf0c-48b1-a4ef-29e22c9ca81f@intel.com> (raw)
In-Reply-To: <3291198.aeNJFYEL58@jkrzyszt-mobl2.ger.corp.intel.com>
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@intel.com>
>> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@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);
>> -}
>>
>
next prev parent reply other threads:[~2024-03-27 4:30 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-26 12:48 [PATCH] drm/i915/hwmon: Remove i915_hwmon_unregister() during driver unbind Badal Nilawar
2024-03-26 21:28 ` Krzysztofik, Janusz
2024-03-27 4:30 ` Nilawar, Badal [this message]
2024-03-27 9:15 ` Krzysztofik, Janusz
2024-03-27 20:37 ` Dixit, Ashutosh
2024-03-28 10:06 ` Krzysztofik, Janusz
2024-03-29 3:47 ` Dixit, Ashutosh
2024-03-27 1:14 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2024-03-27 1:28 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-03-27 19:37 ` [PATCH] " Dixit, Ashutosh
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=b132ded3-bf0c-48b1-a4ef-29e22c9ca81f@intel.com \
--to=badal.nilawar@intel.com \
--cc=anshuman.gupta@intel.com \
--cc=ashutosh.dixit@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=janusz.krzysztofik@intel.com \
--cc=matthew.d.roper@intel.com \
--cc=rodrigo.vivi@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.