Hi Badal,
On Wednesday, 27 March 2024 05:30:17 CET Nilawar, Badal wrote:
>
> 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
For me, that still doesn't explain why you think that i915->hwmon reset to
NULL on i915 driver unregister can be the root cause of the reported UAF in
hwmon sysfs and this patch is going to fix that UAF issue. I can see no
references to i915->hwmon in that code, and even then, that's not NULL what is
reported here as non-canonical address. The code is using a no longer valid
pointer to an already freed (and poisoned) memory.
>
> >
> > 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.
If both uncore and hwmon are managed resources of i915 device then how can you
predict which of them is released first?
Thanks,
Janusz
>
> 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);
> >> -}
> >>
> >
>
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
