Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Marc Zyngier]

Hi Thierry,

On 13/03/2019 16:34, Thierry Reding wrote:
> On Wed, Mar 13, 2019 at 02:20:41PM +0000, Marc Zyngier wrote:

[...]

>> Sure, but look at the result:
>>
>> - you remove your gic-pm module
>> - the MMIO mapping disappears
>> - the GIC data structures *are still live*
>> - a driver does a disable_irq() on an interrupt routed to this block
>> (because nothing has taken the interrupts away, as far as the kernel is
>> concerned)
>> - ...
>> - profit! (or kernel panic, your choice)
> 
> I suppose we could use device links to model the dependency, but perhaps
> it's not worth it for something like the GIC. The AGIC is somewhat of an
> outlier because it serves a fairly encapsulated part of the system, so
> it would be manageable to make it unloadable.

The problem is that the GIC is required so early that it cannot be a
real device (devices require sysfs, which requires the VFS, which has
indirect dependencies in the scheduler, which needs the timer, which
needs interrupts -- I've been pondering turning that upside down for
years, but life is too short).

> 
>> Even better if something else in the system has mapped anything that
>> ends up in the same vmalloc range. Congratulations, you have now
>> corrupted unsuspecting memory. This reminds me of the e1000 corruption
>> bug. Great stuff.
> 
> Can you elaborate on this point? How would unloading a driver cause
> memory corruption in another driver's mapped memory? I've never heard of
> this before, so I want to make sure I understand what to look out for in
> the future.

The MMIO ranges get their VA as part of the vmalloc space. If you free
that space (iounmap), you allow it to be reused by another mapping if
the hole you've created fits the new ioremap. Now the GIC driver (which,
remember, still has references to that VA space all over the place)
unexpectedly pokes into another driver's MMIO space.

But it can be worse. You could have loaded a module instead, which also
ends up in the vmalloc region. And now the GIC driver is writing to the
module code or data. Or anything that gets allocated using vmalloc.

> 
>> So for the whole thing, NAK. You don't pull an irqchip from under the
>> kernel's feet.
> 
> Maybe we should also set the driver's .suppress_bind_attrs = true while
> at it, to prevent anyone from trying to force unbind the driver via
> sysfs?

Would that prevent a 'modprobe -r'? With the current approach, anything
that allows the driver to be removed is potentially fatal.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...