From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S933248AbcIBPza (ORCPT <rfc822;w@1wt.eu>);
        Fri, 2 Sep 2016 11:55:30 -0400
Received: from Galois.linutronix.de ([146.0.238.70]:54569 "EHLO
        Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S932582AbcIBPzY (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 2 Sep 2016 11:55:24 -0400
Date: Fri, 2 Sep 2016 17:53:04 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Sebastian Frias <sf84@laposte.net>
cc: Marc Zyngier <marc.zyngier@arm.com>,
        Jason Cooper <jason@lakedaemon.net>,
        LKML <linux-kernel@vger.kernel.org>, Mason <slash.tmp@free.fr>
Subject: Re: [PATCH] genirq: Generic chip: verify irqs_per_chip <= 32
In-Reply-To: <57B31D94.5040701@laposte.net>
Message-ID: <alpine.DEB.2.20.1609021749570.5647@nanos>
References: <57B31D94.5040701@laposte.net>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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