[PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use

[PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use

[Marek Vasut]

The irq_data_get_irq_chip() can return NULL. If the kernel accesses
chip->irq_get_irqchip_state without checking whether chip is valid,
we get a crash. Fix this by checking whether chip is not NULL before
using it.

Fixes: 1b7047edfcfb ("genirq: Allow the irqchip state of an IRQ to be save/restored")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
To: linux-arm-kernel@lists.infradead.org
---
NOTE: I don't know whether this is a correct fix. Maybe the
      irq_data_get_irq_chip() should never return NULL, and
      I have some other issue?
---
 kernel/irq/manage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fe40c658f86f..509128ab21ea 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -2663,7 +2663,7 @@ int __irq_get_irqchip_state(struct irq_data *data, enum irqchip_irq_state which,
 
 	do {
 		chip = irq_data_get_irq_chip(data);
-		if (chip->irq_get_irqchip_state)
+		if (chip && chip->irq_get_irqchip_state)
 			break;
 #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
 		data = data->parent_data;
@@ -2740,7 +2740,7 @@ int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
 
 	do {
 		chip = irq_data_get_irq_chip(data);
-		if (chip->irq_set_irqchip_state)
+		if (chip && chip->irq_set_irqchip_state)
 			break;
 #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
 		data = data->parent_data;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use

[Thomas Gleixner]

Marek Vasut <marex@denx.de> writes:

> The irq_data_get_irq_chip() can return NULL. If the kernel accesses
> chip->irq_get_irqchip_state without checking whether chip is valid,
> we get a crash. Fix this by checking whether chip is not NULL before
> using it.
>
> Fixes: 1b7047edfcfb ("genirq: Allow the irqchip state of an IRQ to be save/restored")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> NOTE: I don't know whether this is a correct fix. Maybe the
>       irq_data_get_irq_chip() should never return NULL, and
>       I have some other issue?

What's the callchain?

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use

[Marek Vasut]

On 5/7/20 7:30 PM, Thomas Gleixner wrote:
> Marek Vasut <marex@denx.de> writes:
> 
>> The irq_data_get_irq_chip() can return NULL. If the kernel accesses
>> chip->irq_get_irqchip_state without checking whether chip is valid,
>> we get a crash. Fix this by checking whether chip is not NULL before
>> using it.
>>
>> Fixes: 1b7047edfcfb ("genirq: Allow the irqchip state of an IRQ to be save/restored")
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> NOTE: I don't know whether this is a correct fix. Maybe the
>>       irq_data_get_irq_chip() should never return NULL, and
>>       I have some other issue?
> 
> What's the callchain?

Hmm, I'm currently unable to replicate it on linux-next, but on 5.4.39 I
get what's at the end of the email.

On next I just noticed I get i2c: Transfer while suspended, which is
what I suspect would be the real root cause of my problem, and why
irq_data_get_irq_chip() returns NULL?

 8<--- cut here ---
 Unable to handle kernel NULL pointer dereference at virtual address
00000070
 pgd = d06053c1
 [00000070] *pgd=fb2ae835
 Internal error: Oops: 17 [#1] SMP ARM
 Modules linked in:
 CPU: 1 PID: 134 Comm: sh Not tainted 5.4.39-00040-gbfd890984358 #3
 Hardware name: STM32 (Device Tree Support)
 PC is at __irq_get_irqchip_state+0x4/0x30
 LR is at __synchronize_hardirq+0x7c/0xe8
 pc : [<c0166758>]    lr : [<c0166800>]    psr: a0000093
 sp : ed8bddb8  ip : 0000000f  fp : 00000000
 r10: eeedcd68  r9 : c0e0ee04  r8 : eeedcd14
 r7 : eeedcd68  r6 : 00000001  r5 : 40000013  r4 : eeedcd00
 r3 : 00000000  r2 : ed8bddbb  r1 : 00000001  r0 : eeef5f40
 Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
 Control: 10c5387d  Table: eef1c06a  DAC: 00000051
 Process sh (pid: 134, stack limit = 0xd4728d7d)
 Stack: (0xed8bddb8 to 0xed8be000)
 dda0:                                                       00edcd00
c0e04e48
 ddc0: eeedcd00 00000001 0000004d 40000013 c0e0ef40 c01668f0 00000fff
c0e04e48
 dde0: eeedc700 00000001 0000004d c0e04e48 40000013 eeedcd00 00000001
c016cba0
 de00: 00000001 c0e8d714 c0ed0498 00000002 c0e9494c 00000001 c0e8d5bc
00000004
 de20: 00000000 c04e3388 00000002 00000002 c0e8a44c 00000000 c0ed0498
00000001
 de40: c0e9494c 00000001 c0e8d5bc 00000004 00000000 c015f174 2e9b7000
c0162a48
 de60: ed8bde74 c0e04e48 00000000 00000000 c0ed0498 00000001 c0e8d5bc
c094b61f
 de80: c0e94960 c015f6f4 00000007 c0e04e48 eef5c1c3 00000003 00000001
eef5c1c0
 dea0: 00000004 c015e100 00000004 eef5c1c0 eef4c780 ed8bdf78 eef4c790
00000051
 dec0: 00000004 c029a424 00000000 00000000 00000004 00000000 ee9e6540
00000004
 dee0: c029a300 ed8bdf78 ed8bc000 c02301c8 eef1c000 eef1c000 00000000
00000000
 df00: 00000000 00000000 00000000 eef1a03c 00000000 c0e04e48 eeb25a00
00075ff4
 df20: ed8bdfb0 eeebb1e0 00000054 80000007 eef1a040 c015b6ac 00075ff4
c0112a30
 df40: c0101204 c0e04e48 ee9e6540 00000000 ee9e6540 00000004 001d2730
c0231658
 df60: ee9e6540 001d2730 ed8bdf78 ed8bdf84 00000004 c02317fc 00000000
00000000
 df80: 00000000 ee9e6540 00000000 c0e04e48 001ceeac 00000004 001d2730
00000004
 dfa0: c0101204 c0101000 001ceeac 00000004 00000001 001d2730 00000004
00000000
 dfc0: 001ceeac 00000004 001d2730 00000004 00000001 00000002 00000020
00000000
 dfe0: 00000001 be830660 0000c1d0 00008e0c 60000010 00000001 00000000
00000000
 [<c0166758>] (__irq_get_irqchip_state) from [<c0166800>]
(__synchronize_hardirq+0x7c/0xe8)
 [<c0166800>] (__synchronize_hardirq) from [<c01668f0>]
(synchronize_irq+0x2c/0x9c)
 [<c01668f0>] (synchronize_irq) from [<c016cba0>]
(suspend_device_irqs+0xd8/0xf4)
 [<c016cba0>] (suspend_device_irqs) from [<c04e3388>]
(dpm_suspend_noirq+0x18/0x194)
 [<c04e3388>] (dpm_suspend_noirq) from [<c015f174>]
(suspend_devices_and_enter+0x170/0x514)
 [<c015f174>] (suspend_devices_and_enter) from [<c015f6f4>]
(pm_suspend+0x1dc/0x278)
 [<c015f6f4>] (pm_suspend) from [<c015e100>] (state_store+0x9c/0xcc)
 [<c015e100>] (state_store) from [<c029a424>] (kernfs_fop_write+0x124/0x1e0)
 [<c029a424>] (kernfs_fop_write) from [<c02301c8>] (__vfs_write+0x2c/0xe8)
 [<c02301c8>] (__vfs_write) from [<c0231658>] (vfs_write+0x98/0xbc)
 [<c0231658>] (vfs_write) from [<c02317fc>] (ksys_write+0x74/0xc4)
 [<c02317fc>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
 Exception stack(0xed8bdfa8 to 0xed8bdff0)
 dfa0:                   001ceeac 00000004 00000001 001d2730 00000004
00000000
 dfc0: 001ceeac 00000004 001d2730 00000004 00000001 00000002 00000020
00000000
 dfe0: 00000001 be830660 0000c1d0 00008e0c
 Code: e8bd8010 c094f6b4 c094f6ee e5903010 (e5933070)
 ---[ end trace 0c491ff303550b8d ]---

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use

[Thomas Gleixner]

Marek,

Marek Vasut <marex@denx.de> writes:
> On 5/7/20 7:30 PM, Thomas Gleixner wrote:
>> Marek Vasut <marex@denx.de> writes:
>>> NOTE: I don't know whether this is a correct fix. Maybe the
>>>       irq_data_get_irq_chip() should never return NULL, and
>>>       I have some other issue?
>> 
>> What's the callchain?
>
> Hmm, I'm currently unable to replicate it on linux-next, but on 5.4.39 I
> get what's at the end of the email.
>
> On next I just noticed I get i2c: Transfer while suspended, which is
> what I suspect would be the real root cause of my problem, and why
> irq_data_get_irq_chip() returns NULL?

Looks like.

>  Unable to handle kernel NULL pointer dereference at virtual address 00000070
>  PC is at __irq_get_irqchip_state+0x4/0x30
>  LR is at __synchronize_hardirq+0x7c/0xe8
>  [<c0166758>] (__irq_get_irqchip_state) from [<c0166800>] (__synchronize_hardirq+0x7c/0xe8)
>  [<c0166800>] (__synchronize_hardirq) from [<c01668f0>] (synchronize_irq+0x2c/0x9c)
>  [<c01668f0>] (synchronize_irq) from [<c016cba0>] (suspend_device_irqs+0xd8/0xf4)
>  [<c016cba0>] (suspend_device_irqs) from [<c04e3388>] (dpm_suspend_noirq+0x18/0x194)
>  [<c04e3388>] (dpm_suspend_noirq) from [<c015f174>] (suspend_devices_and_enter+0x170/0x514)
>  [<c015f174>] (suspend_devices_and_enter) from [<c015f6f4>] (pm_suspend+0x1dc/0x278)
>  [<c015f6f4>] (pm_suspend) from [<c015e100>] (state_store+0x9c/0xcc)
>  [<c015e100>] (state_store) from [<c029a424>] (kernfs_fop_write+0x124/0x1e0)
>  [<c029a424>] (kernfs_fop_write) from [<c02301c8>] (__vfs_write+0x2c/0xe8)
>  [<c02301c8>] (__vfs_write) from [<c0231658>] (vfs_write+0x98/0xbc)
>  [<c0231658>] (vfs_write) from [<c02317fc>] (ksys_write+0x74/0xc4)
>  [<c02317fc>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x54)

I assume that the i2c controller in question tears down the interrupt on
suspend. The changelog of that i2c driver should give you a few hints.

Thanks,

        tglx


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use

[Marek Vasut]

On 5/7/20 11:51 PM, Thomas Gleixner wrote:
> Marek,
> 
> Marek Vasut <marex@denx.de> writes:
>> On 5/7/20 7:30 PM, Thomas Gleixner wrote:
>>> Marek Vasut <marex@denx.de> writes:
>>>> NOTE: I don't know whether this is a correct fix. Maybe the
>>>>       irq_data_get_irq_chip() should never return NULL, and
>>>>       I have some other issue?
>>>
>>> What's the callchain?
>>
>> Hmm, I'm currently unable to replicate it on linux-next, but on 5.4.39 I
>> get what's at the end of the email.
>>
>> On next I just noticed I get i2c: Transfer while suspended, which is
>> what I suspect would be the real root cause of my problem, and why
>> irq_data_get_irq_chip() returns NULL?
> 
> Looks like.
> 
>>  Unable to handle kernel NULL pointer dereference at virtual address 00000070
>>  PC is at __irq_get_irqchip_state+0x4/0x30
>>  LR is at __synchronize_hardirq+0x7c/0xe8
>>  [<c0166758>] (__irq_get_irqchip_state) from [<c0166800>] (__synchronize_hardirq+0x7c/0xe8)
>>  [<c0166800>] (__synchronize_hardirq) from [<c01668f0>] (synchronize_irq+0x2c/0x9c)
>>  [<c01668f0>] (synchronize_irq) from [<c016cba0>] (suspend_device_irqs+0xd8/0xf4)
>>  [<c016cba0>] (suspend_device_irqs) from [<c04e3388>] (dpm_suspend_noirq+0x18/0x194)
>>  [<c04e3388>] (dpm_suspend_noirq) from [<c015f174>] (suspend_devices_and_enter+0x170/0x514)
>>  [<c015f174>] (suspend_devices_and_enter) from [<c015f6f4>] (pm_suspend+0x1dc/0x278)
>>  [<c015f6f4>] (pm_suspend) from [<c015e100>] (state_store+0x9c/0xcc)
>>  [<c015e100>] (state_store) from [<c029a424>] (kernfs_fop_write+0x124/0x1e0)
>>  [<c029a424>] (kernfs_fop_write) from [<c02301c8>] (__vfs_write+0x2c/0xe8)
>>  [<c02301c8>] (__vfs_write) from [<c0231658>] (vfs_write+0x98/0xbc)
>>  [<c0231658>] (vfs_write) from [<c02317fc>] (ksys_write+0x74/0xc4)
>>  [<c02317fc>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
> 
> I assume that the i2c controller in question tears down the interrupt on
> suspend. The changelog of that i2c driver should give you a few hints.

All right, so I found out the root cause is already fixed in next, and
just needs to be backported to stable. I'll ping the patch author about
that.

It's this patch:
69269446ccbf ("mailbox: stm32-ipcc: Update wakeup management")

I also need to revisit the regulator suspend topic next, that seems to
be a separate issue after all.

Sorry for the noise.

That said, do you want to take this patch to add the missing check
anyway or is there a reason the check is missing ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use

[Thomas Gleixner]

Marek,

Marek Vasut <marex@denx.de> writes:
> On 5/7/20 11:51 PM, Thomas Gleixner wrote:
> All right, so I found out the root cause is already fixed in next, and
> just needs to be backported to stable. I'll ping the patch author about
> that.
>
> It's this patch:
> 69269446ccbf ("mailbox: stm32-ipcc: Update wakeup management")
>
> I also need to revisit the regulator suspend topic next, that seems to
> be a separate issue after all.
>
> Sorry for the noise.

Nothing to be sorry about.

> That said, do you want to take this patch to add the missing check
> anyway or is there a reason the check is missing ?

The reason is probably that chip == NULL is unexpected at least from the
initial callers.

Adding a check for robustness is a good thing, but it should be done
slightly different.

	do {
		chip = irq_data_get_irq_chip(data);
                if (WARN_ON_ONCE(!chip))
                	return -ENODEV;

Simply because to alert the developer that this call is at the wrong
place.

Care to refresh it?

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use

[Marek Vasut]

On 5/13/20 10:49 PM, Thomas Gleixner wrote:
> Marek,
> 
> Marek Vasut <marex@denx.de> writes:
>> On 5/7/20 11:51 PM, Thomas Gleixner wrote:
>> All right, so I found out the root cause is already fixed in next, and
>> just needs to be backported to stable. I'll ping the patch author about
>> that.
>>
>> It's this patch:
>> 69269446ccbf ("mailbox: stm32-ipcc: Update wakeup management")
>>
>> I also need to revisit the regulator suspend topic next, that seems to
>> be a separate issue after all.
>>
>> Sorry for the noise.
> 
> Nothing to be sorry about.
> 
>> That said, do you want to take this patch to add the missing check
>> anyway or is there a reason the check is missing ?
> 
> The reason is probably that chip == NULL is unexpected at least from the
> initial callers.
> 
> Adding a check for robustness is a good thing, but it should be done
> slightly different.
> 
> 	do {
> 		chip = irq_data_get_irq_chip(data);
>                 if (WARN_ON_ONCE(!chip))
>                 	return -ENODEV;
> 
> Simply because to alert the developer that this call is at the wrong
> place.
> 
> Care to refresh it?

Yep, done.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use

[Alexandre Torgue]

Hi Marek

On 5/10/20 4:49 PM, Marek Vasut wrote:
> On 5/7/20 11:51 PM, Thomas Gleixner wrote:
>> Marek,
>>
>> Marek Vasut <marex@denx.de> writes:
>>> On 5/7/20 7:30 PM, Thomas Gleixner wrote:
>>>> Marek Vasut <marex@denx.de> writes:
>>>>> NOTE: I don't know whether this is a correct fix. Maybe the
>>>>>        irq_data_get_irq_chip() should never return NULL, and
>>>>>        I have some other issue?
>>>>
>>>> What's the callchain?
>>>
>>> Hmm, I'm currently unable to replicate it on linux-next, but on 5.4.39 I
>>> get what's at the end of the email.
>>>
>>> On next I just noticed I get i2c: Transfer while suspended, which is
>>> what I suspect would be the real root cause of my problem, and why
>>> irq_data_get_irq_chip() returns NULL?
>>
>> Looks like.
>>
>>>   Unable to handle kernel NULL pointer dereference at virtual address 00000070
>>>   PC is at __irq_get_irqchip_state+0x4/0x30
>>>   LR is at __synchronize_hardirq+0x7c/0xe8
>>>   [<c0166758>] (__irq_get_irqchip_state) from [<c0166800>] (__synchronize_hardirq+0x7c/0xe8)
>>>   [<c0166800>] (__synchronize_hardirq) from [<c01668f0>] (synchronize_irq+0x2c/0x9c)
>>>   [<c01668f0>] (synchronize_irq) from [<c016cba0>] (suspend_device_irqs+0xd8/0xf4)
>>>   [<c016cba0>] (suspend_device_irqs) from [<c04e3388>] (dpm_suspend_noirq+0x18/0x194)
>>>   [<c04e3388>] (dpm_suspend_noirq) from [<c015f174>] (suspend_devices_and_enter+0x170/0x514)
>>>   [<c015f174>] (suspend_devices_and_enter) from [<c015f6f4>] (pm_suspend+0x1dc/0x278)
>>>   [<c015f6f4>] (pm_suspend) from [<c015e100>] (state_store+0x9c/0xcc)
>>>   [<c015e100>] (state_store) from [<c029a424>] (kernfs_fop_write+0x124/0x1e0)
>>>   [<c029a424>] (kernfs_fop_write) from [<c02301c8>] (__vfs_write+0x2c/0xe8)
>>>   [<c02301c8>] (__vfs_write) from [<c0231658>] (vfs_write+0x98/0xbc)
>>>   [<c0231658>] (vfs_write) from [<c02317fc>] (ksys_write+0x74/0xc4)
>>>   [<c02317fc>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
>>
>> I assume that the i2c controller in question tears down the interrupt on
>> suspend. The changelog of that i2c driver should give you a few hints.
> 
> All right, so I found out the root cause is already fixed in next, and
> just needs to be backported to stable. I'll ping the patch author about
> that.
> 
> It's this patch:
> 69269446ccbf ("mailbox: stm32-ipcc: Update wakeup management")

Just to inform you that I got same issue some times ago. As far I 
understood, issue was related to the fact that we used 
"dedicated_wakeup" API for Exti interrupt. Those Exti interrupts had no 
parent which seems to generate this issue (sorry for the lake of details).

Adding a check is a good thing, but note that I have patches in my 
backlog which change a bit hw abstraction done in Exti irqchip driver.
To better fit with HW, each Exti wakeup interrupt will have a GIC irq 
parent (and issue is no more observed).

regards
Alex



> I also need to revisit the regulator suspend topic next, that seems to
> be a separate issue after all.
> 
> Sorry for the noise.
> 
> That said, do you want to take this patch to add the missing check
> anyway or is there a reason the check is missing ?
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel