Re: [PATCH v7 14/34] pinctrl: airoha: fix potential kenel panic in IRQ handling code

[Lorenzo Bianconi]

> airoha_irq_unmask(), airoha_irq_mask(), airoha_irq_type() functions
> gets invalid pointers when initialize gpiochip and pinctrl variables.
> Generally this should lead to kernel panic.
> 
> Details:
> 
>     gpiochip = irq_data_get_irq_chip_data(data);
> 
> will initialize gpiochip variable with data->chip_data value. This value
> initialized inside gpiochip_irq_map() function
> 
>     static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
> 			        irq_hw_number_t hwirq)
>     {
> 	struct gpio_chip *gc = d->host_data;
>         ...
> 	irq_set_chip_data(irq, gc);
> 	...
>     }
> 
> Thus gpiochip variable of 'struct airoha_pinctrl_gpiochip *' type will be
> initialized with a pointer to unrelated variable of 'struct gpio_chip'
> type.
> 
> pinctrl pointer derived from the gpiochip variable
> 
>     pinctrl = container_of(gpiochip, struct airoha_pinctrl, gpiochip);
> 
> thus it will get wrong value as well.
> 
> So any access to the data pointed by gpiochip and pinctrl variables is
> extremelly dangerous.
> 
> This patch implements correct logic of getting gpiochip and pinctrl
> pointers.
> 
> Fixes: 1c8ace2d0725 ("pinctrl: airoha: Add support for EN7581 SoC")
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@xxxxxxxxx>
> ---
>  drivers/pinctrl/airoha/pinctrl-airoha.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index faad5d3ada31c..6cf4ed5976fb0 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
> @@ -2569,18 +2569,17 @@ static int airoha_gpio_direction_output(struct gpio_chip *chip,
>  /* irq callbacks */
>  static void airoha_irq_unmask(struct irq_data *data)
>  {
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +	struct airoha_pinctrl *pinctrl = gpiochip_get_data(gc);
> +	struct airoha_pinctrl_gpiochip *gpiochip = &pinctrl->gpiochip;
>  	u8 offset = data->hwirq % AIROHA_REG_GPIOCTRL_NUM_PIN;
>  	u8 index = data->hwirq / AIROHA_REG_GPIOCTRL_NUM_PIN;
>  	u32 mask = GENMASK(2 * offset + 1, 2 * offset);
> -	struct airoha_pinctrl_gpiochip *gpiochip;
> -	struct airoha_pinctrl *pinctrl;
>  	u32 val = BIT(2 * offset);
>  
> -	gpiochip = irq_data_get_irq_chip_data(data);

I agree the proposed approach is more standard and I am fine with it, but can
you please provide more details about how it can panic?

gpio_chip is the first element of airoha_pinctrl_gpiochip so it is fine to cast
irq_data_get_irq_chip_data() return value to airoha_pinctrl_gpiochip, right?

Regards,
Lorenzo

>  	if (WARN_ON_ONCE(data->hwirq >= ARRAY_SIZE(gpiochip->irq_type)))
>  		return;
>  
> -	pinctrl = container_of(gpiochip, struct airoha_pinctrl, gpiochip);
>  	switch (gpiochip->irq_type[data->hwirq]) {
>  	case IRQ_TYPE_LEVEL_LOW:
>  		val = val << 1;
> @@ -2606,14 +2605,12 @@ static void airoha_irq_unmask(struct irq_data *data)
>  
>  static void airoha_irq_mask(struct irq_data *data)
>  {
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +	struct airoha_pinctrl *pinctrl = gpiochip_get_data(gc);
> +	struct airoha_pinctrl_gpiochip *gpiochip = &pinctrl->gpiochip;
>  	u8 offset = data->hwirq % AIROHA_REG_GPIOCTRL_NUM_PIN;
>  	u8 index = data->hwirq / AIROHA_REG_GPIOCTRL_NUM_PIN;
>  	u32 mask = GENMASK(2 * offset + 1, 2 * offset);
> -	struct airoha_pinctrl_gpiochip *gpiochip;
> -	struct airoha_pinctrl *pinctrl;
> -
> -	gpiochip = irq_data_get_irq_chip_data(data);
> -	pinctrl = container_of(gpiochip, struct airoha_pinctrl, gpiochip);
>  
>  	regmap_clear_bits(pinctrl->regmap, gpiochip->level[index], mask);
>  	regmap_clear_bits(pinctrl->regmap, gpiochip->edge[index], mask);
> @@ -2621,9 +2618,10 @@ static void airoha_irq_mask(struct irq_data *data)
>  
>  static int airoha_irq_type(struct irq_data *data, unsigned int type)
>  {
> -	struct airoha_pinctrl_gpiochip *gpiochip;
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +	struct airoha_pinctrl *pinctrl = gpiochip_get_data(gc);
> +	struct airoha_pinctrl_gpiochip *gpiochip = &pinctrl->gpiochip;
>  
> -	gpiochip = irq_data_get_irq_chip_data(data);
>  	if (data->hwirq >= ARRAY_SIZE(gpiochip->irq_type))
>  		return -EINVAL;
>  
> -- 
> 2.53.0
>

Attachment: signature.asc
Description: PGP signature