[PATCH] genirq: Generic chip: verify irqs_per_chip <= 32

[PATCH] genirq: Generic chip: verify irqs_per_chip <= 32

[Sebastian Frias]

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.

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.

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
 include/linux/irq.h       | 20 +++++++++++++++-----
 kernel/irq/generic-chip.c | 16 ++++++++--------
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index b52424e..2a527e6 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -916,11 +916,21 @@ void irq_remove_generic_chip(struct irq_chip_generic *gc, u32 msk,
 			     unsigned int clr, unsigned int set);
 
 struct irq_chip_generic *irq_get_domain_generic_chip(struct irq_domain *d, unsigned int hw_irq);
-int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
-				   int num_ct, const char *name,
-				   irq_flow_handler_t handler,
-				   unsigned int clr, unsigned int set,
-				   enum irq_gc_flags flags);
+
+#define irq_alloc_domain_generic_chips(d, irqs_per_chip, num_ct, name,	\
+				       handler,	clr, set, flags)	\
+	({								\
+		MAYBE_BUILD_BUG_ON(irqs_per_chip > 32);			\
+		__irq_alloc_domain_generic_chips(d, irqs_per_chip, num_ct, \
+						 name, handler, clr, set, \
+						 flags);		\
+	})
+
+int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				     int num_ct, const char *name,
+				     irq_flow_handler_t handler,
+				     unsigned int clr, unsigned int set,
+				     enum irq_gc_flags flags);
 
 
 static inline struct irq_chip_type *irq_data_get_chip_type(struct irq_data *d)
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index abd286a..302ab659 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -260,9 +260,9 @@ irq_gc_init_mask_cache(struct irq_chip_generic *gc, enum irq_gc_flags flags)
 }
 
 /**
- * irq_alloc_domain_generic_chip - Allocate generic chips for an irq domain
+ * __irq_alloc_domain_generic_chip - Allocate generic chips for an irq domain
  * @d:			irq domain for which to allocate chips
- * @irqs_per_chip:	Number of interrupts each chip handles
+ * @irqs_per_chip:	Number of interrupts each chip handles (max 32)
  * @num_ct:		Number of irq_chip_type instances associated with this
  * @name:		Name of the irq chip
  * @handler:		Default flow handler associated with these chips
@@ -270,11 +270,11 @@ irq_gc_init_mask_cache(struct irq_chip_generic *gc, enum irq_gc_flags flags)
  * @set:		IRQ_* bits to set in the mapping function
  * @gcflags:		Generic chip specific setup flags
  */
-int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
-				   int num_ct, const char *name,
-				   irq_flow_handler_t handler,
-				   unsigned int clr, unsigned int set,
-				   enum irq_gc_flags gcflags)
+int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				     int num_ct, const char *name,
+				     irq_flow_handler_t handler,
+				     unsigned int clr, unsigned int set,
+				     enum irq_gc_flags gcflags)
 {
 	struct irq_domain_chip_generic *dgc;
 	struct irq_chip_generic *gc;
@@ -326,7 +326,7 @@ int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
 	d->name = name;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(irq_alloc_domain_generic_chips);
+EXPORT_SYMBOL_GPL(__irq_alloc_domain_generic_chips);
 
 /**
  * irq_get_domain_generic_chip - Get a pointer to the generic chip of a hw_irq
-- 
1.7.11.2

[PATCH] genirq: Generic chip: verify irqs_per_chip <= 32

[Sebastian Frias]

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.

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.

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
 include/linux/irq.h       | 20 +++++++++++++++-----
 kernel/irq/generic-chip.c | 16 ++++++++--------
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index b52424e..2a527e6 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -916,11 +916,21 @@ void irq_remove_generic_chip(struct irq_chip_generic *gc, u32 msk,
 			     unsigned int clr, unsigned int set);
 
 struct irq_chip_generic *irq_get_domain_generic_chip(struct irq_domain *d, unsigned int hw_irq);
-int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
-				   int num_ct, const char *name,
-				   irq_flow_handler_t handler,
-				   unsigned int clr, unsigned int set,
-				   enum irq_gc_flags flags);
+
+#define irq_alloc_domain_generic_chips(d, irqs_per_chip, num_ct, name,	\
+				       handler,	clr, set, flags)	\
+	({								\
+		MAYBE_BUILD_BUG_ON(irqs_per_chip > 32);			\
+		__irq_alloc_domain_generic_chips(d, irqs_per_chip, num_ct, \
+						 name, handler, clr, set, \
+						 flags);		\
+	})
+
+int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				     int num_ct, const char *name,
+				     irq_flow_handler_t handler,
+				     unsigned int clr, unsigned int set,
+				     enum irq_gc_flags flags);
 
 
 static inline struct irq_chip_type *irq_data_get_chip_type(struct irq_data *d)
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index abd286a..302ab659 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -260,9 +260,9 @@ irq_gc_init_mask_cache(struct irq_chip_generic *gc, enum irq_gc_flags flags)
 }
 
 /**
- * irq_alloc_domain_generic_chip - Allocate generic chips for an irq domain
+ * __irq_alloc_domain_generic_chip - Allocate generic chips for an irq domain
  * @d:			irq domain for which to allocate chips
- * @irqs_per_chip:	Number of interrupts each chip handles
+ * @irqs_per_chip:	Number of interrupts each chip handles (max 32)
  * @num_ct:		Number of irq_chip_type instances associated with this
  * @name:		Name of the irq chip
  * @handler:		Default flow handler associated with these chips
@@ -270,11 +270,11 @@ irq_gc_init_mask_cache(struct irq_chip_generic *gc, enum irq_gc_flags flags)
  * @set:		IRQ_* bits to set in the mapping function
  * @gcflags:		Generic chip specific setup flags
  */
-int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
-				   int num_ct, const char *name,
-				   irq_flow_handler_t handler,
-				   unsigned int clr, unsigned int set,
-				   enum irq_gc_flags gcflags)
+int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				     int num_ct, const char *name,
+				     irq_flow_handler_t handler,
+				     unsigned int clr, unsigned int set,
+				     enum irq_gc_flags gcflags)
 {
 	struct irq_domain_chip_generic *dgc;
 	struct irq_chip_generic *gc;
@@ -326,7 +326,7 @@ int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
 	d->name = name;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(irq_alloc_domain_generic_chips);
+EXPORT_SYMBOL_GPL(__irq_alloc_domain_generic_chips);
 
 /**
  * irq_get_domain_generic_chip - Get a pointer to the generic chip of a hw_irq
-- 
1.7.11.2

Re: [PATCH] genirq: Generic chip: verify irqs_per_chip <= 32

[Thomas Gleixner]

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

[tip:irq/core] genirq/generic_chip: Verify irqs_per_chip <= 32

[tip-bot for Sebastian Frias]

Commit-ID:  895d3b95ed05f72a94f69ab52cb313915a6b889f
Gitweb:     http://git.kernel.org/tip/895d3b95ed05f72a94f69ab52cb313915a6b889f
Author:     Sebastian Frias <sf84@laposte.net>
AuthorDate: Tue, 16 Aug 2016 16:05:08 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 2 Sep 2016 18:06:50 +0200

genirq/generic_chip: Verify irqs_per_chip <= 32

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 will result in
unexpected results.

Provide a wrapper with a MAYBE_BUILD_BUG_ON(nrirqs >= 32) to catch such
cases.

[ tglx: Reduced changelog to the essential information ]

Signed-off-by: Sebastian Frias <sf84@laposte.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mason <slash.tmp@free.fr>
Cc: Jason Cooper <jason@lakedaemon.net>
Link: http://lkml.kernel.org/r/57B31D94.5040701@laposte.net
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 include/linux/irq.h       | 18 +++++++++++++-----
 kernel/irq/generic-chip.c | 16 ++++++++--------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index b52424e..9a96860 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -916,12 +916,20 @@ void irq_remove_generic_chip(struct irq_chip_generic *gc, u32 msk,
 			     unsigned int clr, unsigned int set);
 
 struct irq_chip_generic *irq_get_domain_generic_chip(struct irq_domain *d, unsigned int hw_irq);
-int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
-				   int num_ct, const char *name,
-				   irq_flow_handler_t handler,
-				   unsigned int clr, unsigned int set,
-				   enum irq_gc_flags flags);
 
+int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				     int num_ct, const char *name,
+				     irq_flow_handler_t handler,
+				     unsigned int clr, unsigned int set,
+				     enum irq_gc_flags flags);
+
+#define irq_alloc_domain_generic_chips(d, irqs_per_chip, num_ct, name,	\
+				       handler,	clr, set, flags)	\
+({									\
+	MAYBE_BUILD_BUG_ON(irqs_per_chip > 32);				\
+	__irq_alloc_domain_generic_chips(d, irqs_per_chip, num_ct,	\
+					 handler, clr, set, flags);	\
+})
 
 static inline struct irq_chip_type *irq_data_get_chip_type(struct irq_data *d)
 {
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index a3a3920..ee32870 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -260,9 +260,9 @@ irq_gc_init_mask_cache(struct irq_chip_generic *gc, enum irq_gc_flags flags)
 }
 
 /**
- * irq_alloc_domain_generic_chip - Allocate generic chips for an irq domain
+ * __irq_alloc_domain_generic_chip - Allocate generic chips for an irq domain
  * @d:			irq domain for which to allocate chips
- * @irqs_per_chip:	Number of interrupts each chip handles
+ * @irqs_per_chip:	Number of interrupts each chip handles (max 32)
  * @num_ct:		Number of irq_chip_type instances associated with this
  * @name:		Name of the irq chip
  * @handler:		Default flow handler associated with these chips
@@ -270,11 +270,11 @@ irq_gc_init_mask_cache(struct irq_chip_generic *gc, enum irq_gc_flags flags)
  * @set:		IRQ_* bits to set in the mapping function
  * @gcflags:		Generic chip specific setup flags
  */
-int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
-				   int num_ct, const char *name,
-				   irq_flow_handler_t handler,
-				   unsigned int clr, unsigned int set,
-				   enum irq_gc_flags gcflags)
+int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				     int num_ct, const char *name,
+				     irq_flow_handler_t handler,
+				     unsigned int clr, unsigned int set,
+				     enum irq_gc_flags gcflags)
 {
 	struct irq_domain_chip_generic *dgc;
 	struct irq_chip_generic *gc;
@@ -326,7 +326,7 @@ int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
 	d->name = name;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(irq_alloc_domain_generic_chips);
+EXPORT_SYMBOL_GPL(__irq_alloc_domain_generic_chips);
 
 static struct irq_chip_generic *
 __irq_get_domain_generic_chip(struct irq_domain *d, unsigned int hw_irq)

[tip:irq/core] genirq/generic_chip: Verify irqs_per_chip <= 32

[tip-bot for Sebastian Frias]

Commit-ID:  f88eecfe2f22b2790e7527c0aaec14ea175919de
Gitweb:     http://git.kernel.org/tip/f88eecfe2f22b2790e7527c0aaec14ea175919de
Author:     Sebastian Frias <sf84@laposte.net>
AuthorDate: Tue, 16 Aug 2016 16:05:08 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 2 Sep 2016 20:20:59 +0200

genirq/generic_chip: Verify irqs_per_chip <= 32

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 will result in
unexpected results.

Provide a wrapper with a MAYBE_BUILD_BUG_ON(nrirqs >= 32) to catch such
cases.

[ tglx: Reduced changelog to the essential information ]

Signed-off-by: Sebastian Frias <sf84@laposte.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mason <slash.tmp@free.fr>
Cc: Jason Cooper <jason@lakedaemon.net>
Link: http://lkml.kernel.org/r/57B31D94.5040701@laposte.net
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/irq.h       | 18 +++++++++++++-----
 kernel/irq/generic-chip.c | 16 ++++++++--------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index b52424e..6039867 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -916,12 +916,20 @@ void irq_remove_generic_chip(struct irq_chip_generic *gc, u32 msk,
 			     unsigned int clr, unsigned int set);
 
 struct irq_chip_generic *irq_get_domain_generic_chip(struct irq_domain *d, unsigned int hw_irq);
-int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
-				   int num_ct, const char *name,
-				   irq_flow_handler_t handler,
-				   unsigned int clr, unsigned int set,
-				   enum irq_gc_flags flags);
 
+int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				     int num_ct, const char *name,
+				     irq_flow_handler_t handler,
+				     unsigned int clr, unsigned int set,
+				     enum irq_gc_flags flags);
+
+#define irq_alloc_domain_generic_chips(d, irqs_per_chip, num_ct, name,	\
+				       handler,	clr, set, flags)	\
+({									\
+	MAYBE_BUILD_BUG_ON(irqs_per_chip > 32);				\
+	__irq_alloc_domain_generic_chips(d, irqs_per_chip, num_ct, name,\
+					 handler, clr, set, flags);	\
+})
 
 static inline struct irq_chip_type *irq_data_get_chip_type(struct irq_data *d)
 {
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index a3a3920..ee32870 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -260,9 +260,9 @@ irq_gc_init_mask_cache(struct irq_chip_generic *gc, enum irq_gc_flags flags)
 }
 
 /**
- * irq_alloc_domain_generic_chip - Allocate generic chips for an irq domain
+ * __irq_alloc_domain_generic_chip - Allocate generic chips for an irq domain
  * @d:			irq domain for which to allocate chips
- * @irqs_per_chip:	Number of interrupts each chip handles
+ * @irqs_per_chip:	Number of interrupts each chip handles (max 32)
  * @num_ct:		Number of irq_chip_type instances associated with this
  * @name:		Name of the irq chip
  * @handler:		Default flow handler associated with these chips
@@ -270,11 +270,11 @@ irq_gc_init_mask_cache(struct irq_chip_generic *gc, enum irq_gc_flags flags)
  * @set:		IRQ_* bits to set in the mapping function
  * @gcflags:		Generic chip specific setup flags
  */
-int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
-				   int num_ct, const char *name,
-				   irq_flow_handler_t handler,
-				   unsigned int clr, unsigned int set,
-				   enum irq_gc_flags gcflags)
+int __irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				     int num_ct, const char *name,
+				     irq_flow_handler_t handler,
+				     unsigned int clr, unsigned int set,
+				     enum irq_gc_flags gcflags)
 {
 	struct irq_domain_chip_generic *dgc;
 	struct irq_chip_generic *gc;
@@ -326,7 +326,7 @@ int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
 	d->name = name;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(irq_alloc_domain_generic_chips);
+EXPORT_SYMBOL_GPL(__irq_alloc_domain_generic_chips);
 
 static struct irq_chip_generic *
 __irq_get_domain_generic_chip(struct irq_domain *d, unsigned int hw_irq)