linux-kernel - Re: [PATCH] genirq: Generic chip: verify irqs_per

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]

Date:   Fri, 2 Sep 2016 17:53:04 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Sebastian Frias <sf84@...oste.net>
cc:     Marc Zyngier <marc.zyngier@....com>,
        Jason Cooper <jason@...edaemon.net>,
        LKML <linux-kernel@...r.kernel.org>, Mason <slash.tmp@...e.fr>
Subject: Re: [PATCH] genirq: Generic chip: verify irqs_per_chip <= 32

On Tue, 16 Aug 2016, Sebastian Frias wrote:
> Most (if not all) code here implicitly assumes that the maximum number of
> IRQs per chip will be 32, and thus uses 'u32' or 'unsigned long' for many
> tasks (for example "struct irq_data" declares its 'mask' field as 'u32',
> and "struct irq_chip_generic" declares its 'installed' field as 'unsigned
> long')
> 
> However, there is no check to verify that irqs_per_chip is <= 32.
> Hence, calling irq_alloc_domain_generic_chips() with a bigger value would
> result in unexpected results depending on how the 'mask' is accessed later
> on.
> For example, if set_bit() is used on the 'installed' field of "struct
> irq_chip_generic", it would result in the next field (in this case
> the 'unused' field) being overwritten, because set_bit() is designed to
> treat its parameter as a field of bits of arbitrary size organised as
> "unsigned long" words.

We really do not need examples for the potential wreckage. Your explanation
that the code has implict assumptions about the maximum number of
interrupts per chip is sufficient. We all know what out of bound access can
cause.

> This patch renames irq_alloc_domain_generic_chips() to
> __irq_alloc_domain_generic_chips() and creates a macro to replace it.
> The macro uses MAYBE_BUILD_BUG_ON to check the irqs_per_chip parameter to
> stop compilation (if the compiler can resolve the parameter to a constant
> at compile time) or to warn during run-time, if the parameter given is
> bigger than 32.

There is no point in explaining the implementation in the changelog. It's
obvious from the patch itself.

I'll fix that up as well.

Thanks,

	tglx