LinuxLists.cc - [PATCH] genirq:Dereference desc after null pointer check

2018-07-17 07:00:05

Subject: [PATCH] genirq:Dereference desc after null pointer check

* Remove unchecked dereferencing of *desc & assigning it local irq
variable

* Move the assignement after proper check of *desc for NULL pointer.

Signed-off-by: RAGHU Halharvi <[email protected]>
---
kernel/irq/manage.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index facfecfc543c..064d98e5ae32 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1557,7 +1557,7 @@ EXPORT_SYMBOL_GPL(setup_irq);
*/
static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
{
- unsigned irq = desc->irq_data.irq;
+ unsigned int irq;
struct irqaction *action, **action_ptr;
unsigned long flags;

@@ -1566,6 +1566,7 @@ static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
if (!desc)
return NULL;

+ irq = desc->irq_data.irq;
mutex_lock(&desc->request_mutex);
chip_bus_lock(desc);
raw_spin_lock_irqsave(&desc->lock, flags);
--
2.17.1

2018-07-17 07:34:51

by Raghu Halharvi

[permalink] [raw]

Subject: [PATCH] genirq:Dereference desc after null pointer check

* Remove unchecked dereferencing of *desc & assigning it local irq
variable
* Move the assignement after proper check of *desc for NULL pointer.

Signed-off-by: RAGHU Halharvi <[email protected]>
---
kernel/irq/manage.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index facfecfc543c..33c77eb4fdb6 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1557,15 +1557,16 @@ EXPORT_SYMBOL_GPL(setup_irq);
*/
static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
{
- unsigned irq = desc->irq_data.irq;
+ unsigned int irq;
struct irqaction *action, **action_ptr;
unsigned long flags;

- WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);

if (!desc)
return NULL;

+ irq = desc->irq_data.irq;
+ WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);
mutex_lock(&desc->request_mutex);
chip_bus_lock(desc);
raw_spin_lock_irqsave(&desc->lock, flags);
--
2.17.1

2018-07-17 07:52:53

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [PATCH] genirq:Dereference desc after null pointer check

On Tue, 17 Jul 2018, RAGHU Halharvi wrote:

> * Remove unchecked dereferencing of *desc & assigning it local irq
> variable
> * Move the assignement after proper check of *desc for NULL pointer.

Sorry no. The NULL pointer check is pointless because the call sites
already do it before calling that function.

Thanks,

tglx

2018-07-17 12:27:57

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH] genirq:Dereference desc after null pointer check

Hi RAGHU,

Thank you for
url: git checkout 3ebb02d34b vim +/irq +1571
d3c60047 Thomas Gleixner cbf94f06 Magnus Damm cbf94f06 Magnus Damm cbf94f06 Magnus Damm ^1da177e Linus Torvalds 83ac4ca9 Uwe Kleine K?nig ^1da177e Linus Torvalds 3ebb02d3 RAGHU Halharvi f17c7545 Ingo Molnar ^1da177e Linus Torvalds ^1da177e Linus Torvalds ae88a23b Ingo Molnar Not initialized.

7d94f7ca Yinghai Lu 7d94f7ca Yinghai Lu f21cfb25 Magnus Damm ^1da177e Linus Torvalds 3ebb02d3 RAGHU Halharvi 9114014c Thomas Gleixner abc7e40c Thomas Gleixner 239007b8 Thomas Gleixner ae88a23b Ingo Molnar ae88a23b Ingo Molnar ae88a23b Ingo Molnar ae88a23b Ingo Molnar ae88a23b Ingo Molnar f17c7545 Ingo Molnar ^1da177e Linus Torvalds f17c7545 Ingo Molnar ^1da177e Linus Torvalds ae88a23b Ingo Molnar ae88a23b Ingo Molnar 239007b8 Thomas Gleixner abc7e40c Thomas Gleixner 19d39a38 Thomas Gleixner f21cfb25 Magnus Damm ae88a23b Ingo Molnar ^1da177e Linus Torvalds 8316e381 Ingo Molnar ae88a23b Ingo Molnar f17c7545 Ingo Molnar ae88a23b Ingo Molnar ^1da177e Linus Torvalds ae88a23b Ingo Molnar f17c7545 Ingo Molnar dbce706e Paolo cab303be Thomas Gleixner cab303be Thomas Gleixner ae88a23b Ingo Molnar c1bacbae Thomas Gleixner e9849777 Thomas Gleixner 46999238 Thomas Gleixner c1bacbae Thomas Gleixner 3aa551c9 Thomas Gleixner
:::::: The code :::::: ae88a23b32fa7e0d
:::::: TO: Ingo :::::: CC: Ingo
---
0-DAY kernel test infrastructure https://github.com/0day-ci/linux/commits/RAGHU-Halharvi/genirq-Dereference-desc-after-null-pointer-check/20180717-150842
__free_irq() error: uninitialized symbol 'irq'.
hub.com/0day-ci/linux/commit/3ebb02d34bcb1c8111a79856326bca85fb3321eb">https://github.com/0day-ci/linux/commit/3ebb02d34bcb1c8111a79856326bca85fb3321eb
href="https://github.com/0day-ci/linux">https://github.com/0day-ci/linux
cb1c8111a79856326bca85fb3321eb
kernel/irq/manage.c
2008-10-16 1560
2009-03-12 1561 /*
2009-03-12 1562 * Internal function to unregister an irqaction - used to free
2009-03-12 1563 * regular and special interrupts that are part of the architecture.
2005-04-16 1564 */
2018-03-19 1565 static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
2005-04-16 1566 {
2018-07-17 1567 unsigned int irq;
2009-02-17 1568 struct irqaction *action, **action_ptr;
2005-04-16 1569 unsigned long flags;
2005-04-16 1570
2009-02-15 @1571 WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);
^^^
2008-08-19 1572
2008-08-19 1573 if (!desc)
2009-03-12 1574 return NULL;
2005-04-16 1575
2018-07-17 1576 irq = desc->irq_data.irq;
2017-06-29 1577 mutex_lock(&desc->request_mutex);
2015-12-13 1578 chip_bus_lock(desc);
2009-11-17 1579 raw_spin_lock_irqsave(&desc->lock, flags);
2009-02-15 1580
2009-02-15 1581 /*
2009-02-15 1582 * There can be multiple actions per IRQ descriptor, find the right
2009-02-15 1583 * one based on the dev_id:
2009-02-15 1584 */
2009-02-17 1585 action_ptr = &desc->action;
2005-04-16 1586 for (;;) {
2009-02-17 1587 action = *action_ptr;
2005-04-16 1588
2009-02-15 1589 if (!action) {
2009-02-15 1590 WARN(1, "Trying to free already-free IRQ %d\n", irq);
2009-11-17 1591 raw_spin_unlock_irqrestore(&desc->lock, flags);
2015-12-13 1592 chip_bus_sync_unlock(desc);
2017-07-11 1593 mutex_unlock(&desc->request_mutex);
2009-03-12 1594 return NULL;
2009-02-15 1595 }
2005-04-16 1596
2009-02-17 1597 if (action->dev_id == dev_id)
2009-02-15 1598 break;
2009-02-17 1599 action_ptr = &action->next;
2009-02-15 1600 }
2005-04-16 1601
2009-02-15 1602 /* Found it - now remove it from the list of entries: */
2009-02-17 1603 *action_ptr = action->next;
'Blaisorblade' Giarrusso 2005-06-21 1604
2014-08-28 1605 irq_pm_remove_action(desc, action);
2014-08-28 1606
2009-02-15 1607 /* If this was the last handler, shut down the IRQ line: */
2014-03-08 1608 if (!desc->action) {
2015-10-09 1609 irq_settings_clr_disable_unlazy(desc);
2011-02-02 1610 irq_shutdown(desc);
2014-03-08 1611 }
2009-03-23 1612
at line 1571 was first introduced by commit
c9fa7ce735966e68eb41b0bc irq: refactor and clean up the free_irq() code flow
Molnar <[email protected]>
Molnar <[email protected]>
Open Source Technology Center
.01.org/pipermail/kbuild-all">https://lists.01.org/pipermail/kbuild-all Intel Corporation

2018-07-17 13:25:49

by Raghu Halharvi

[permalink] [raw]

Subject: Re: [PATCH] genirq:Dereference desc after null pointer check

Yes Dan, I should have done better.

Also Tom suggested to get rid of NULL pointer check as its redundant
because callee does that before calling __free_irq.

On Tue, Jul 17, 2018 at 03:25:57PM +0300, Dan Carpenter wrote:
> Hi RAGHU,
>
> Thank you for the patch! Perhaps something to improve:
>
> url: https://github.com/0day-ci/linux/commits/RAGHU-Halharvi/genirq-Dereference-desc-after-null-pointer-check/20180717-150842
>
> smatch warnings:
> kernel/irq/manage.c:1571 __free_irq() error: uninitialized symbol 'irq'.
>
> # https://github.com/0day-ci/linux/commit/3ebb02d34bcb1c8111a79856326bca85fb3321eb
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 3ebb02d34bcb1c8111a79856326bca85fb3321eb
> vim +/irq +1571 kernel/irq/manage.c
>
> d3c60047 Thomas Gleixner 2008-10-16 1560
> cbf94f06 Magnus Damm 2009-03-12 1561 /*
> cbf94f06 Magnus Damm 2009-03-12 1562 * Internal function to unregister an irqaction - used to free
> cbf94f06 Magnus Damm 2009-03-12 1563 * regular and special interrupts that are part of the architecture.
> ^1da177e Linus Torvalds 2005-04-16 1564 */
> 83ac4ca9 Uwe Kleine K?nig 2018-03-19 1565 static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
> ^1da177e Linus Torvalds 2005-04-16 1566 {
> 3ebb02d3 RAGHU Halharvi 2018-07-17 1567 unsigned int irq;
> f17c7545 Ingo Molnar 2009-02-17 1568 struct irqaction *action, **action_ptr;
> ^1da177e Linus Torvalds 2005-04-16 1569 unsigned long flags;
> ^1da177e Linus Torvalds 2005-04-16 1570
> ae88a23b Ingo Molnar 2009-02-15 @1571 WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);
> ^^^
> Not initialized.
>
> 7d94f7ca Yinghai Lu 2008-08-19 1572
> 7d94f7ca Yinghai Lu 2008-08-19 1573 if (!desc)
> f21cfb25 Magnus Damm 2009-03-12 1574 return NULL;
> ^1da177e Linus Torvalds 2005-04-16 1575
> 3ebb02d3 RAGHU Halharvi 2018-07-17 1576 irq = desc->irq_data.irq;
> 9114014c Thomas Gleixner 2017-06-29 1577 mutex_lock(&desc->request_mutex);
> abc7e40c Thomas Gleixner 2015-12-13 1578 chip_bus_lock(desc);
> 239007b8 Thomas Gleixner 2009-11-17 1579 raw_spin_lock_irqsave(&desc->lock, flags);
> ae88a23b Ingo Molnar 2009-02-15 1580
> ae88a23b Ingo Molnar 2009-02-15 1581 /*
> ae88a23b Ingo Molnar 2009-02-15 1582 * There can be multiple actions per IRQ descriptor, find the right
> ae88a23b Ingo Molnar 2009-02-15 1583 * one based on the dev_id:
> ae88a23b Ingo Molnar 2009-02-15 1584 */
> f17c7545 Ingo Molnar 2009-02-17 1585 action_ptr = &desc->action;
> ^1da177e Linus Torvalds 2005-04-16 1586 for (;;) {
> f17c7545 Ingo Molnar 2009-02-17 1587 action = *action_ptr;
> ^1da177e Linus Torvalds 2005-04-16 1588
> ae88a23b Ingo Molnar 2009-02-15 1589 if (!action) {
> ae88a23b Ingo Molnar 2009-02-15 1590 WARN(1, "Trying to free already-free IRQ %d\n", irq);
> 239007b8 Thomas Gleixner 2009-11-17 1591 raw_spin_unlock_irqrestore(&desc->lock, flags);
> abc7e40c Thomas Gleixner 2015-12-13 1592 chip_bus_sync_unlock(desc);
> 19d39a38 Thomas Gleixner 2017-07-11 1593 mutex_unlock(&desc->request_mutex);
> f21cfb25 Magnus Damm 2009-03-12 1594 return NULL;
> ae88a23b Ingo Molnar 2009-02-15 1595 }
> ^1da177e Linus Torvalds 2005-04-16 1596
> 8316e381 Ingo Molnar 2009-02-17 1597 if (action->dev_id == dev_id)
> ae88a23b Ingo Molnar 2009-02-15 1598 break;
> f17c7545 Ingo Molnar 2009-02-17 1599 action_ptr = &action->next;
> ae88a23b Ingo Molnar 2009-02-15 1600 }
> ^1da177e Linus Torvalds 2005-04-16 1601
> ae88a23b Ingo Molnar 2009-02-15 1602 /* Found it - now remove it from the list of entries: */
> f17c7545 Ingo Molnar 2009-02-17 1603 *action_ptr = action->next;
> dbce706e Paolo 'Blaisorblade' Giarrusso 2005-06-21 1604
> cab303be Thomas Gleixner 2014-08-28 1605 irq_pm_remove_action(desc, action);
> cab303be Thomas Gleixner 2014-08-28 1606
> ae88a23b Ingo Molnar 2009-02-15 1607 /* If this was the last handler, shut down the IRQ line: */
> c1bacbae Thomas Gleixner 2014-03-08 1608 if (!desc->action) {
> e9849777 Thomas Gleixner 2015-10-09 1609 irq_settings_clr_disable_unlazy(desc);
> 46999238 Thomas Gleixner 2011-02-02 1610 irq_shutdown(desc);
> c1bacbae Thomas Gleixner 2014-03-08 1611 }
> 3aa551c9 Thomas Gleixner 2009-03-23 1612
>
> :::::: The code at line 1571 was first introduced by commit
> :::::: ae88a23b32fa7e0dc9fa7ce735966e68eb41b0bc irq: refactor and clean up the free_irq() code flow
>
> :::::: TO: Ingo Molnar <[email protected]>
> :::::: CC: Ingo Molnar <[email protected]>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation