[-next] irqchip/tango: Fix potential NULL pointer dereference
diff mbox series

Message ID 20190129080122.20392-1-yuehaibing@huawei.com
State New
Headers show
Series
  • [-next] irqchip/tango: Fix potential NULL pointer dereference
Related show

Commit Message

YueHaibing Jan. 29, 2019, 8:01 a.m. UTC
There is a potential NULL pointer dereference in case kzalloc()
fails and returns NULL.

Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs SMP86xx/SMP87xx interrupt controller")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/irqchip/irq-tango.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Marc Zyngier Jan. 29, 2019, 8:55 a.m. UTC | #1
On Tue, 29 Jan 2019 08:01:22 +0000,
YueHaibing <yuehaibing@huawei.com> wrote:
> 
> There is a potential NULL pointer dereference in case kzalloc()
> fails and returns NULL.
> 
> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs SMP86xx/SMP87xx interrupt controller")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
>  drivers/irqchip/irq-tango.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
> index ae28d86..a63b828 100644
> --- a/drivers/irqchip/irq-tango.c
> +++ b/drivers/irqchip/irq-tango.c
> @@ -191,6 +191,8 @@ static int __init tangox_irq_init(void __iomem *base, struct resource *baseres,
>  		panic("%pOFn: failed to get address", node);
>  
>  	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
>  	chip->ctl = res.start - baseres->start;
>  	chip->base = base;
>  

This is a commendable effort, but given that the whole error handling
of this driver is just to simply panic, I have the ugly feeling that
this lack of check is more a feature than a bug... Not that I like it,
but at least it is consistent.

If you're going to change that, I'd recommend you overhaul the whole
thing.

Thanks,

	M.
Måns Rullgård Jan. 29, 2019, 12:20 p.m. UTC | #2
Marc Zyngier <marc.zyngier@arm.com> writes:

> On Tue, 29 Jan 2019 08:01:22 +0000,
> YueHaibing <yuehaibing@huawei.com> wrote:
>> 
>> There is a potential NULL pointer dereference in case kzalloc()
>> fails and returns NULL.
>> 
>> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs SMP86xx/SMP87xx interrupt controller")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>>  drivers/irqchip/irq-tango.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
>> index ae28d86..a63b828 100644
>> --- a/drivers/irqchip/irq-tango.c
>> +++ b/drivers/irqchip/irq-tango.c
>> @@ -191,6 +191,8 @@ static int __init tangox_irq_init(void __iomem *base, struct resource *baseres,
>>  		panic("%pOFn: failed to get address", node);
>>  
>>  	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>> +	if (!chip)
>> +		return -ENOMEM;
>>  	chip->ctl = res.start - baseres->start;
>>  	chip->base = base;
>>  
>
> This is a commendable effort, but given that the whole error handling
> of this driver is just to simply panic, I have the ugly feeling that
> this lack of check is more a feature than a bug... Not that I like it,
> but at least it is consistent.

That seemed to be the norm for irqchip drivers when I wrote this one,
and a fair number of them still panic on errors during init.  There's
really not much else that can sanely be done since nothing will work
without irq handling.

As for the error return added by this patch, nothing checks it, so a
failure would merely result in the irqchip being silently skipped and
nothing working.  Propagating the error back to of_irq_init() also has
no effect, not even a warning.  Besides, kzalloc() is extremely unlikely
to fail at this stage, and if it does, you have much bigger problems.
YueHaibing Jan. 30, 2019, 6:25 a.m. UTC | #3
On 2019/1/29 20:20, Måns Rullgård wrote:
> Marc Zyngier <marc.zyngier@arm.com> writes:
> 
>> On Tue, 29 Jan 2019 08:01:22 +0000,
>> YueHaibing <yuehaibing@huawei.com> wrote:
>>>
>>> There is a potential NULL pointer dereference in case kzalloc()
>>> fails and returns NULL.
>>>
>>> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs SMP86xx/SMP87xx interrupt controller")
>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>> ---
>>>  drivers/irqchip/irq-tango.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
>>> index ae28d86..a63b828 100644
>>> --- a/drivers/irqchip/irq-tango.c
>>> +++ b/drivers/irqchip/irq-tango.c
>>> @@ -191,6 +191,8 @@ static int __init tangox_irq_init(void __iomem *base, struct resource *baseres,
>>>  		panic("%pOFn: failed to get address", node);
>>>  
>>>  	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>> +	if (!chip)
>>> +		return -ENOMEM;
>>>  	chip->ctl = res.start - baseres->start;
>>>  	chip->base = base;
>>>  
>>
>> This is a commendable effort, but given that the whole error handling
>> of this driver is just to simply panic, I have the ugly feeling that
>> this lack of check is more a feature than a bug... Not that I like it,
>> but at least it is consistent.
> 
> That seemed to be the norm for irqchip drivers when I wrote this one,
> and a fair number of them still panic on errors during init.  There's
> really not much else that can sanely be done since nothing will work
> without irq handling.
> 
> As for the error return added by this patch, nothing checks it, so a
> failure would merely result in the irqchip being silently skipped and
> nothing working.  Propagating the error back to of_irq_init() also has
> no effect, not even a warning.  Besides, kzalloc() is extremely unlikely
> to fail at this stage, and if it does, you have much bigger problems.

Thanks for your comment.

>

Patch
diff mbox series

diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
index ae28d86..a63b828 100644
--- a/drivers/irqchip/irq-tango.c
+++ b/drivers/irqchip/irq-tango.c
@@ -191,6 +191,8 @@  static int __init tangox_irq_init(void __iomem *base, struct resource *baseres,
 		panic("%pOFn: failed to get address", node);
 
 	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
 	chip->ctl = res.start - baseres->start;
 	chip->base = base;