Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 25 May 2020 11:55:33 +0100
Message-ID: <87tv04z3kq.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Oscar Carter <oscar.carter@gmx.com>
Cc:     Kees Cook <keescook@chromium.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jason Cooper <jason@lakedaemon.net>,
        kernel-hardening@lists.openwall.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] drivers/irqchip: Remove function callback casts
In-Reply-To: <20200524162220.10186-1-oscar.carter@gmx.com>
References: <20200524162220.10186-1-oscar.carter@gmx.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, 24 May 2020 17:22:20 +0100,
Oscar Carter <oscar.carter@gmx.com> wrote:
> 
> In an effort to enable -Wcast-function-type in the top-level Makefile to
> support Control Flow Integrity builds, remove all the function callback
> casts.
> 
> To do this, create a new macro called ACPI_DECLARE_SUBTABLE_PROBE_ENTRY
> to initialize the acpi_probe_entry struct using the probe_subtbl field
> instead of the probe_table field. Then, modify the IRQCHIP_ACPI_DECLARE
> macro to use this new defined macro.
> 
> Even though these two commented fields are part of a union, this is
> necessary to avoid function cast mismatches. That is, due to the
> IRQCHIP_ACPI_DECLARE invocations use as last parameter a function with
> the protoype "int (*func)(struct acpi_subtable_header *, const unsigned
> long)" it's necessary that this macro initialize the probe_subtbl field
> of the acpi_probe_entry struct and not the probe_table field.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
> Changelog v1->v2
> - Add more details in the commit changelog to clarify the changes (Marc
>   Zyngier)
> - Declare a new macro called ACPI_DECLARE_SUBTABLE_PROBE_ENTRY (Marc
>   Zyngier)
> - In the IRQCHIP_ACPI_DECLARE use the new defined macro (Marc Zyngier)
> 
>  include/linux/acpi.h    | 11 +++++++++++

You now need to Cc the ACPI maintainers.

>  include/linux/irqchip.h |  5 +++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index d661cd0ee64d..fed49b276a90 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1154,6 +1154,17 @@ struct acpi_probe_entry {
>  			.driver_data = data, 				\
>  		   }
> 
> +#define ACPI_DECLARE_SUBTABLE_PROBE_ENTRY(table, name, table_id,	\
> +					  subtable, valid, data, fn)	\
> +	static const struct acpi_probe_entry __acpi_probe_##name	\
> +		__used __section(__##table##_acpi_probe_table) = {	\
> +			.id = table_id,					\
> +			.type = subtable,				\
> +			.subtable_valid = valid,			\
> +			.probe_subtbl = (acpi_tbl_entry_handler)fn,	\

It strikes me that under the guise of removing function casts, you are
actually adding one! And this cast is actually hiding all sorts of
sins (remove it, and see things exploding). I've fixed it with the
patch below (ACPI is such a mess of data structure case...).

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index d7006ef18a0d..3870e9d4d3a8 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -2117,7 +2117,7 @@ static void __init gic_acpi_setup_kvm_info(void)
 }
 
 static int __init
-gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
+gic_acpi_init(union acpi_subtable_headers *header, const unsigned long end)
 {
 	struct acpi_madt_generic_distributor *dist;
 	struct fwnode_handle *domain_handle;
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 30ab623343d3..fc431857ce90 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1593,7 +1593,7 @@ static void __init gic_acpi_setup_kvm_info(void)
 	gic_set_kvm_info(&gic_v2_kvm_info);
 }
 
-static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
+static int __init gic_v2_acpi_init(union acpi_subtable_headers *header,
 				   const unsigned long end)
 {
 	struct acpi_madt_generic_distributor *dist;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index fed49b276a90..4f4ddbfce3d3 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1150,7 +1150,7 @@ struct acpi_probe_entry {
 			.id = table_id,					\
 			.type = subtable,				\
 			.subtable_valid = valid,			\
-			.probe_table = (acpi_tbl_table_handler)fn,	\
+			.probe_table = fn,				\
 			.driver_data = data, 				\
 		   }
 
@@ -1161,7 +1161,7 @@ struct acpi_probe_entry {
 			.id = table_id,					\
 			.type = subtable,				\
 			.subtable_valid = valid,			\
-			.probe_subtbl = (acpi_tbl_entry_handler)fn,	\
+			.probe_subtbl = fn,				\
 			.driver_data = data,				\
 		}
 
Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.