Re: [PATCH v2] HID: hid-oxp: fix UAF on pending work in remove()

"Shengzhuo Wei" <[email protected]>
Newsgroups org.kernel.vger.stable,org.kernel.vger.linux-input,org.kernel.vger.linux-kernel
Message-ID <anLtyBB-Vq2D6REw@pve>
Hi Derek, Dmitry,

Derek wrote:
> Out of curiosity, do you have a device available to test as well? If
> so, which model do you have?

I don't have any OneXPlayer hardware. I found this bug by creating a
gen2 OXP device through /dev/uhid and tearing it down under KASAN in
QEMU, not on a real device, and I'm not deeply familiar with the HID
subsystem overall.

On disable_delayed_work_sync() I seem to be caught between your two
positions. Derek:

> I don't think the bug fix switching to disable_delayed_work_sync is
> worth the side effects without first addressing the global drvdata
> issue. That would introduce a true regression to fix a theoretical
> logic bug. I would rather you either switch to the devm_alloc drvdata
> first or hold that change until I'm able to do it later myself.

Dmitry:

> If there is possibility to have multiple instances then this driver is
> FUBAR in the current shape: one device instance fires up work items
> for another device. So holding the patch makes no sense IMO.

I'm not well placed to own the per-device drvdata refactor: I have no
hardware to validate the multi-interface behavior, and I don't have the
HID-subsystem familiarity to do that rework confidently. So I'd rather
not take that piece on.

Given that, what would you like me to do? The realistic contribution I
can stand behind is a minimal v3 that only fixes the use-after-free I
actually found and verified -- cancel_delayed_work() ->
cancel_delayed_work_sync() in oxp_hid_remove() (the v1 approach),
reverting v2's probe change and dropping disable_delayed_work_sync().
It's KASAN-verified, introduces no regression, and needs no hardware.
The re-arm hardening and the drvdata/attr-ordering rework would then be
Derek's, on real hardware with the hid-msi pattern.

If you'd rather I drop the patch entirely and leave the whole thing
(including the cancel_delayed_work_sync part) to Derek, that's fine
too. I'd like to land this fix if there's a version of it you'd accept
from me, but I defer to your judgment on scope -- I just didn't want to
silently walk away from a bug I reported.

Thanks,
Shengzhuo
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.