[PATCH] irqchip/gic: Check interrupt type validity

[PATCH] irqchip/gic: Check interrupt type validity

[Florian Fainelli]

In case the interrupt property specifies a type parameter that is not
GIC_SPI (0) or GIC_PPIC (1), do not attempt to translate the interrupt
and return -EINVAL instead.

Fixes: f833f57ff254 ("irqchip: Convert all alloc/xlate users from of_node to fwnode")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Marc,

Regardless of whether my attempt to use SGI moves any further, this
seems appropriate to do since we should not be trying to translate
incorrectly specified interrupts. Thanks!

 drivers/irqchip/irq-gic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 30ab623343d3..fc47e655618d 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1005,6 +1005,9 @@ static int gic_irq_domain_translate(struct irq_domain *d,
 		if (fwspec->param_count < 3)
 			return -EINVAL;
 
+		if (fwspec->param[0] > 1)
+			return -EINVAL;
+
 		/* Get the interrupt number and add 16 to skip over SGIs */
 		*hwirq = fwspec->param[1] + 16;
 
-- 
2.17.1

Re: [PATCH] irqchip/gic: Check interrupt type validity

[Florian Fainelli]

On 10/23/19 12:56 PM, Florian Fainelli wrote:
> In case the interrupt property specifies a type parameter that is not
> GIC_SPI (0) or GIC_PPIC (1), do not attempt to translate the interrupt
> and return -EINVAL instead.
> 
> Fixes: f833f57ff254 ("irqchip: Convert all alloc/xlate users from of_node to fwnode")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Marc,
> 
> Regardless of whether my attempt to use SGI moves any further, this
> seems appropriate to do since we should not be trying to translate
> incorrectly specified interrupts. Thanks!

Ping?

> 
>  drivers/irqchip/irq-gic.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 30ab623343d3..fc47e655618d 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1005,6 +1005,9 @@ static int gic_irq_domain_translate(struct irq_domain *d,
>  		if (fwspec->param_count < 3)
>  			return -EINVAL;
>  
> +		if (fwspec->param[0] > 1)
> +			return -EINVAL;
> +
>  		/* Get the interrupt number and add 16 to skip over SGIs */
>  		*hwirq = fwspec->param[1] + 16;
>  
> 


-- 
Florian

Re: [PATCH] irqchip/gic: Check interrupt type validity

[Marc Zyngier]

On Wed, 23 Oct 2019 12:56:19 -0700
Florian Fainelli <f.fainelli@gmail.com> wrote:

> In case the interrupt property specifies a type parameter that is not
> GIC_SPI (0) or GIC_PPIC (1), do not attempt to translate the interrupt
> and return -EINVAL instead.
> 
> Fixes: f833f57ff254 ("irqchip: Convert all alloc/xlate users from of_node to fwnode")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Marc,
> 
> Regardless of whether my attempt to use SGI moves any further, this
> seems appropriate to do since we should not be trying to translate
> incorrectly specified interrupts. Thanks!
> 
>  drivers/irqchip/irq-gic.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 30ab623343d3..fc47e655618d 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1005,6 +1005,9 @@ static int gic_irq_domain_translate(struct irq_domain *d,
>  		if (fwspec->param_count < 3)
>  			return -EINVAL;
>  
> +		if (fwspec->param[0] > 1)
> +			return -EINVAL;
> +
>  		/* Get the interrupt number and add 16 to skip over SGIs */
>  		*hwirq = fwspec->param[1] + 16;
>  

I'm in two minds about this.

The usual stance is that the kernel is not a validation suite for DT
files, but on the other hand we already do some of that two lines above
(a consequence of kernel and DT binding lockstep development...). Do we
really want to add more of this? Or should we put more effort in static
validation of DT files and actually remove these checks?

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

Re: [PATCH] irqchip/gic: Check interrupt type validity

[Florian Fainelli]

On 11/16/19 5:35 AM, Marc Zyngier wrote:
> On Wed, 23 Oct 2019 12:56:19 -0700
> Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
>> In case the interrupt property specifies a type parameter that is not
>> GIC_SPI (0) or GIC_PPIC (1), do not attempt to translate the interrupt
>> and return -EINVAL instead.
>>
>> Fixes: f833f57ff254 ("irqchip: Convert all alloc/xlate users from of_node to fwnode")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> Marc,
>>
>> Regardless of whether my attempt to use SGI moves any further, this
>> seems appropriate to do since we should not be trying to translate
>> incorrectly specified interrupts. Thanks!
>>
>>  drivers/irqchip/irq-gic.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index 30ab623343d3..fc47e655618d 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -1005,6 +1005,9 @@ static int gic_irq_domain_translate(struct irq_domain *d,
>>  		if (fwspec->param_count < 3)
>>  			return -EINVAL;
>>  
>> +		if (fwspec->param[0] > 1)
>> +			return -EINVAL;
>> +
>>  		/* Get the interrupt number and add 16 to skip over SGIs */
>>  		*hwirq = fwspec->param[1] + 16;
>>  
> 
> I'm in two minds about this.
> 
> The usual stance is that the kernel is not a validation suite for DT
> files, but on the other hand we already do some of that two lines above
> (a consequence of kernel and DT binding lockstep development...). Do we
> really want to add more of this? Or should we put more effort in static
> validation of DT files and actually remove these checks?


Static validation is nice for DTS that live in the tree, if you have a
platform that provides the FDT via firmware, and that firmware is handed
over to a variety of people, who knows what they can do. My inclination
would be to perform that check which is not costly and guarantees we
don't feed garbage to the GIC driver.
-- 
Florian