linux-kernel - Re: [PATCH v2] firewire: ohci: prevent leak of left-over IRQ on unbind

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <wrcvrmxqfy2zfpbcgoy4txqmzcoyptzvctzymztlp55gasu2fg@tyudozxoyvzo>
Date: Sat, 02 Mar 2024 19:48:53 +0000
From: Edmund Raile <edmund.raile@...ton.me>
To: Takashi Sakamoto <o-takashi@...amocchi.jp>
Cc: linux-kernel@...r.kernel.org, linux1394-devel@...ts.sourceforge.net
Subject: Re: [PATCH v2] firewire: ohci: prevent leak of left-over IRQ on unbind

> In my opinion, the devres mechanism releases the allocated memory when
> releasing the data of associated device structure.
> device_release_driver_internal()
> ->__device_release_driver()
>   ->device_unbind_cleanup()
>     (drivers/base/devres.c)
>     ->devres_release_all(dev);
>       ->release_nodes()
>         (kernel/irq/devres.c)
>       ->free_irq()

Looking at __device_release_driver() in drivers/base/dd.c,
device_remove() gets called, leading to dev->bus->remove(dev),
which likely calls our good old friend from the call trace:
pci_device_remove().

> > Call Trace:
> >  ? remove_proc_entry+0x19c/0x1c0
> >  ? __warn+0x81/0x130
> >  ? remove_proc_entry+0x19c/0x1c0
> >  ? report_bug+0x171/0x1a0
> >  ? console_unlock+0x78/0x120
> >  ? handle_bug+0x3c/0x80
> >  ? exc_invalid_op+0x17/0x70
> >  ? asm_exc_invalid_op+0x1a/0x20
> >  ? remove_proc_entry+0x19c/0x1c0
> >  unregister_irq_proc+0xf4/0x120
> >  free_desc+0x3d/0xe0
> >  ? kfree+0x29f/0x2f0
> >  irq_free_descs+0x47/0x70
> >  msi_domain_free_locked.part.0+0x19d/0x1d0
> >  msi_domain_free_irqs_all_locked+0x81/0xc0
> >  pci_free_msi_irqs+0x12/0x40
> >  pci_disable_msi+0x4c/0x60
> >  pci_remove+0x9d/0xc0 [firewire_ohci
> >      01b483699bebf9cb07a3d69df0aa2bee71db1b26]
> >  pci_device_remove+0x37/0xa0
> >  device_release_driver_internal+0x19f/0x200
> >  unbind_store+0xa1/0xb0

Then in ohci.c's pci_remove(), we kill the MSIs, which leads to
the removal of the IRQ, etc.
Back in __device_release_driver(), after device_remove(),
device_unbind_cleanup() is called, leading to free_irq(), but too late.

I think the order of these calls may be our issue but I doubt it
has been done like this without good reason.
That code is 8 years old, someone would have noticed if it had an error.

I could be entirely wrong but the function description in
/kernel/irq/devres.c tells me that function is meant to be used:

> Except for the extra @dev argument, this function takes the
> same arguments and performs the same function as free_irq().
> This function instead of free_irq() should be used to manually
> free IRQs allocated with devm_request_irq().

And while devm_request_irq() has no function description of its own, its
sister devm_request_threaded_irq() mentions this:

> IRQs requested with this function will be
> automatically freed on driver detach.
>
> If an IRQ allocated with this function needs to be freed
> separately, devm_free_irq() must be used.

Should we pull in the maintainers of dd.c for their opinion?

Thank you very much for all the very hard work you do Sakamoto-Sensei!