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

[Mikhail Kshevetskiy]

On 7/27/26 11:31, Lorenzo Bianconi wrote:
>> 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?

Sorry, you are right, the will be no panic unless structure fields will
be reordered.
I'll fix commit message


>
> 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
>>