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.
>
> 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
--
Ville Syrjälä
Intel
